Conversation
…Kind
Improve test coverage for guard_policy.go:
- AllowOnlyPolicy.UnmarshalJSON: 67.7% → 93.5%
- Add error-path tests for all 9 JSON fields (repos, min-integrity,
blocked-users, approval-labels, trusted-users, endorsement-reactions,
disapproval-reactions, disapproval-integrity, endorser-min-integrity)
- Add happy-path tests for the 4 previously-untested fields:
endorsement-reactions, disapproval-reactions, disapproval-integrity,
endorser-min-integrity
- NormalizeScopeKind: 0% → 100%
- Test nil input, empty map, missing scope_kind, lowercase, uppercase,
leading/trailing spaces, non-string value, preservation of other fields,
and immutability of the input map
- GuardPolicy.UnmarshalJSON: coverage improved with invalid-inner-JSON paths
- NormalizeGuardPolicy: 97.8% → 98.9%
- Add tests for endorsement/disapproval dedup and validation logic
- ValidateWriteSinkPolicy: add nil-input test
Overall config package coverage: 92.2% → 93.8%
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds targeted unit tests in internal/config to increase branch/edge-case coverage for guard-policy parsing and normalization code paths, especially around AllowOnlyPolicy.UnmarshalJSON and NormalizeScopeKind.
Changes:
- Added table-driven tests covering
NormalizeScopeKindnormalization behavior and immutability. - Added tests for
GuardPolicy/AllowOnlyPolicyJSON unmarshal error paths and the newer endorsement/disapproval-related fields. - Added tests exercising selected
NormalizeGuardPolicy/ValidateWriteSinkPolicyvalidation and normalization behaviors.
Show a summary per file
| File | Description |
|---|---|
| internal/config/guard_policy_unmarshal_coverage_test.go | New tests to improve coverage of guard-policy unmarshalling/normalization and related validation paths |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 4
| { | ||
| name: "allow-only inner value is invalid JSON", | ||
| json: `{"allow-only": {not valid}}`, | ||
| wantErr: "invalid character", | ||
| }, |
There was a problem hiding this comment.
The JSON literal in this case is not valid JSON, so GuardPolicy.UnmarshalJSON fails in the initial json.Unmarshal(data, &raw) step and never reaches the inner allow-only unmarshal. If the goal is to exercise the inner-object error path, use syntactically valid JSON where allow-only has an invalid type/shape (e.g., a string/number/array) so the top-level parse succeeds and the error comes from unmarshalling allowOnlyRaw into AllowOnlyPolicy.
| name: "write-sink inner value is invalid JSON", | ||
| json: `{"write-sink": {not valid}}`, | ||
| wantErr: "invalid character", |
There was a problem hiding this comment.
Same issue as the prior case: {"write-sink": {not valid}} is invalid top-level JSON, so this doesn't test an inner write-sink unmarshal failure—it's effectively covered by the "invalid top-level JSON" test. To test the json.Unmarshal(writeSinkRaw, &writeSink) failure, make the overall JSON valid but set write-sink to an incompatible type/shape (or an object with an invalid accept type).
| name: "write-sink inner value is invalid JSON", | |
| json: `{"write-sink": {not valid}}`, | |
| wantErr: "invalid character", | |
| name: "write-sink inner value fails WriteSinkPolicy unmarshal", | |
| json: `{"write-sink": "not an object"}`, | |
| wantErr: "cannot unmarshal string", |
| { | ||
| name: "repos field invalid JSON type", | ||
| json: `{"repos": {invalid}, "min-integrity": "none"}`, | ||
| wantErr: "invalid character", | ||
| }, |
There was a problem hiding this comment.
This "repos field invalid JSON type" case uses invalid JSON ({invalid}), so AllowOnlyPolicy.UnmarshalJSON fails before it can iterate fields; it doesn't exercise the repos-specific branch. Also, since Repos is interface{}, unmarshalling any valid JSON value into it won't error—so there isn't a realistic per-field unmarshal error path for repos here. Consider removing this case, or move repos-type validation to a NormalizeGuardPolicy test (e.g., a non-string/non-array repos value should hit the allow-only.repos must be ... error).
| assert.Equal(t, original.MinIntegrity, parsed.MinIntegrity) | ||
| assert.Equal(t, original.BlockedUsers, parsed.BlockedUsers) | ||
| assert.Equal(t, original.ApprovalLabels, parsed.ApprovalLabels) | ||
| assert.Equal(t, original.TrustedUsers, parsed.TrustedUsers) | ||
| assert.Equal(t, original.EndorsementReactions, parsed.EndorsementReactions) |
There was a problem hiding this comment.
This round-trip test doesn't assert anything about Repos. Since Repos is interface{}, it's easy for regressions to slip through (e.g., the slice type/contents changing) while all other assertions still pass. Add an assertion that parsed.Repos deep-equals original.Repos (and/or assert the expected concrete type, like []interface{} with string elements).
|
@copilot update the pr based on this review feedback #4012 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/f16c3573-ed63-4f60-bbba-958a1ca6f6a9 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Updated in commit I addressed the review feedback by switching the inner-unmarshal tests to valid top-level JSON with incompatible inner types, removing the invalid/impossible No UI changes were made (test-only change), so there is no UI screenshot to provide. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
lpcox
deleted the
test/guard-policy-coverage-improvement-bc556122811dd013
branch
Test Coverage Improvement:
guard_policy.goFunctions Analyzed
AllowOnlyPolicy.UnmarshalJSONinternal/configNormalizeScopeKindinternal/configGuardPolicy.UnmarshalJSONinternal/configNormalizeGuardPolicyinternal/configValidateWriteSinkPolicyinternal/configOverall
internal/configpackage: 92.2% → 93.8%Why These Functions?
AllowOnlyPolicy.UnmarshalJSONhad 67.7% coverage despite being a core security-policy parsing function with 9 distinct field branches and error paths. The four newest fields —endorsement-reactions,disapproval-reactions,disapproval-integrity, andendorser-min-integrity— had zero test coverage for both happy paths and error paths.NormalizeScopeKindwas an exported function at 0% coverage in theinternal/configpackage (existing tests live ininternal/serverwhich requires thego-sdkdependency and couldn't run in the CI environment that generated coverage data).Tests Added
TestNormalizeScopeKind— 10 table-driven cases: nil input, empty map, noscope_kindfield, already-lowercase, uppercase, whitespace trimming, mixed case+spaces, non-string value, other fields preserved, immutability of inputTestGuardPolicyUnmarshalJSON_InvalidInnerJSON— error paths whenallow-only/write-sinkinner values contain invalid JSON or fail inner-struct validationTestAllowOnlyPolicyUnmarshalJSON_FieldErrorPaths— error paths for all 9 fields when the JSON value has an incompatible type (repos,min-integrity,blocked-users,approval-labels,trusted-users,endorsement-reactions,disapproval-reactions,disapproval-integrity,endorser-min-integrity)TestAllowOnlyPolicyUnmarshalJSON_EndorsementDisapprovalFields— happy-path parsing for all 4 previously-untested fields, including empty arrays and the full set togetherTestValidateWriteSinkPolicy_NilInput— nil guard inValidateWriteSinkPolicyTestNormalizeGuardPolicy_WriteSinkPath— write-sink path error inNormalizeGuardPolicyTestNormalizeGuardPolicy_EndorsementReactionDedup— deduplication (case-insensitive), empty-entry rejection, valid integrity normalization forendorsement-reactions,disapproval-reactions,disapproval-integrity,endorser-min-integrityTestNormalizeAndValidateScopeArray_NonStringElement— non-string element in repos arrayTestAllowOnlyPolicyUnmarshalJSON_FullRoundTrip— full marshal/unmarshal round-trip with all fieldsCoverage Report
Generated by Test Coverage Improver
Next run will target the next most complex under-tested function
Warning
The following domains were blocked by the firewall during workflow execution:
go.opentelemetry.iogo.yaml.ingolang.orggoogle.golang.orggopkg.ininvalidhostthatdoesnotexist12345.comproxy.golang.orgreleaseassets.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.