test(actionpins): improve spec_test.go quality per testify-expert checklist#46470
Conversation
- Add doc comment to testSHAResolver.ResolveSHA method - Normalise assert.Contains to assert.Containsf in TestSpec_DesignDecision_FormatConsistency - Add EnforcePinned=true + resolver-succeeds case to TestSpec_PublicAPI_ResolveActionPin_EnforcePinned - Add @sha256: format assertion to TestSpec_PublicAPI_GetContainerPin - Add TestSpec_DynamicResolution_EmptySHAFallsThrough for empty-SHA resolver path - Add TestSpec_PublicAPI_ResolveActionPin_NilCtxField for nil ctx.Ctx fallback - Merge TestSpec_PublicAPI_RecordResolutionFailure pair into table-driven test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves actionpins specification tests for resolver edge cases, enforcement, container formats, and auditing.
Changes:
- Adds dynamic-resolution and nil-context coverage.
- Expands pin-format and enforce-mode assertions.
- Consolidates resolution-failure tests.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/spec_test.go |
Strengthens and reorganizes action pin specification tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Medium
| assert.Empty(t, result, "downgraded unresolved result should remain empty") | ||
| require.NoError(t, err, "non-error scenario should not return an error") | ||
| if tt.wantResultSHA != "" { | ||
| assert.Contains(t, result, tt.wantResultSHA, "successful resolution should include the resolved SHA") |
| assert.True(t, strings.HasPrefix(pin.Digest, "sha256:"), "ContainerPin.Digest should use sha256: format, got %q", pin.Digest) | ||
| assert.NotEmpty(t, pin.PinnedImage, "ContainerPin.PinnedImage should be non-empty for a known image") | ||
| assert.Contains(t, pin.PinnedImage, pin.Digest, "PinnedImage should contain the digest") | ||
| assert.Contains(t, pin.PinnedImage, "@sha256:", "PinnedImage should reference the digest using @sha256: notation") |
| require.NoError(t, err) | ||
| require.NotEmpty(t, result) | ||
| }, "nil PinContext.Ctx should fall back to context.Background() without panicking") | ||
| require.NotNil(t, resolver.capturedCtx, "resolver must receive a non-nil context even when PinContext.Ctx is nil") |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot please run the Outstanding review threads:
Please keep the scope focused, update the branch if needed, and summarize the fixes in the PR. Run: https://github.com/github/gh-aw/actions/runs/29662541327
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: test quality improvements in pkg/actionpins/spec_test.go
The overall direction is good — merging two flat tests into a table-driven test, adding missing branches, and normalising assertion style are all sound improvements.
Three existing inline comments flag the highest-signal issues and I agree with all of them:
- Line 329 —
assert.Containsfor the resolved SHA is too loose; a substring match lets malformed results pass. Use the full documented format assertion instead. - Line 588 — Checking
sha256:prefix and@sha256:independently does not validate the canonicalimage@sha256:digeststructure. One exact-match assertion covers both. - Line 744 — The nil-ctx test only verifies some non-nil context was passed; per the documented contract it should specifically be
context.Background(). Assert identity withassert.Sameor compare viaassert.Equal(t, context.Background(), resolver.capturedCtx).
No additional blocking issues found beyond those already raised.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12 AIC · ⌖ 4.33 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (6 tests)
Analysis
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review summary
Three pre-existing review comments correctly flag weak assertions; adding one new finding about incomplete test coverage.
Findings:
- Line 718 (new):
TestSpec_DynamicResolution_EmptySHAFallsThrough— unknown-repo sub-test omitsRecordResolutionFailurefromPinContext. The production guard silently no-ops, so the auditing side-effect goes entirely unverified. See inline comment. - Line 329 (existing):
assert.Containson the enforce-mode success SHA is too weak. - Line 588 (existing): Independent digest checks do not verify the composite format.
- Line 744 (existing):
capturedCtx != nildoes not verifycontext.Background()identity.
All medium-severity test quality issues; no crashes or data-loss bugs.
🔎 Code quality review by PR Code Quality Reviewer · 33 AIC · ⌖ 4.51 AIC · ⊞ 5.6K
Comment /review to run again
| assert.Contains(t, result, latestPin.SHA, "result should use the hardcoded pin SHA") | ||
| }) | ||
|
|
||
| t.Run("empty SHA with nil error falls through and produces empty for unknown repo", func(t *testing.T) { |
There was a problem hiding this comment.
Incomplete coverage: the unknown-repo sub-test never validates whether RecordResolutionFailure was invoked.
💡 Details
The production code in ResolveActionPin (line 342 of actionpins.go) calls recordPinResolutionFailure for the unknown-repo path when a resolver is present. The guard in that helper silently no-ops when RecordResolutionFailure == nil, so the test does not panic — but it also never asserts whether the failure was recorded (or not), leaving a key side-effect completely unverified.
Suggested fix:
t.Run("empty SHA with nil error falls through and produces empty for unknown repo", func(t *testing.T) {
var failures []actionpins.ResolutionFailure
ctx := &actionpins.PinContext{
Resolver: &testSHAResolver{sha: "", err: nil},
Warnings: make(map[string]bool),
RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
failures = append(failures, f)
},
}
result, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx)
require.NoError(t, err)
assert.Empty(t, result, "empty-SHA resolver on unknown repo should produce empty result")
// Verify the failure was recorded with the expected classification.
require.Len(t, failures, 1)
assert.Equal(t, actionpins.ResolutionErrorTypeDynamicResolutionFailed, failures[0].ErrorType)
})Without this, the test is silent about a documented auditing contract.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on assertion precision and test-helper correctness.
📋 Key Themes & Highlights
Key Themes
- Weak
Containsassertions appear in multiple new tests and in existing tests that this PR extends. Several new assertions check that a SHA substring appears anywhere in the result string, which would allow malformed output to pass silently. The pattern is consistent with the issues already flagged by prior review comments on lines 329, 588, and 744. requireinsiderequire.NotPanicsclosure (line 736) — this is a correctness bug:t.FailNow()inside the closure is swallowed as aruntime.Goexit()and misread byNotPanicsas a non-panic exit, masking real failures.
Positive Highlights
- ✅ Merging the two
RecordResolutionFailuretests into a table-driven form is a clean, low-duplication improvement. - ✅ The new
TestSpec_DynamicResolution_EmptySHAFallsThroughtests cover a genuinely tricky edge case well. - ✅ The
assert.*fnormalisation inTestSpec_DesignDecision_FormatConsistencyimproves failure message quality.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.6 AIC · ⌖ 6.33 AIC · ⊞ 6.7K
Comment /matt to run again
| ctx := &actionpins.PinContext{ | ||
| Resolver: &testSHAResolver{sha: "", err: nil}, | ||
| Warnings: make(map[string]bool), | ||
| } |
There was a problem hiding this comment.
[/tdd] assert.Contains(t, result, latestPin.SHA) is a weak assertion — a malformed result like junk<sha> would still pass. Assert the exact documented repo@sha format.
💡 Suggested assertion
assert.True(t, strings.HasPrefix(result, known+"@"), "result should start with repo@sha, got %q", result)
assert.Contains(t, result, latestPin.SHA, "result should contain the hardcoded pin SHA")Or an exact assert.Equal if the full format is stable. This mirrors how TestSpec_PublicAPI_ResolveActionPin_DynamicHappyPath asserts the output contract.
@copilot please address this.
| resolver := &testSHAResolver{sha: testResolvedSHA} | ||
| ctx := &actionpins.PinContext{ | ||
| Ctx: nil, // deliberately nil — should fall back to context.Background() | ||
| Resolver: resolver, |
There was a problem hiding this comment.
[/tdd] require inside a require.NotPanics callback silently swallows assertion failures — require.NoError calls t.FailNow() → runtime.Goexit(), which NotPanics interprets as a successful non-panic; the test passes while hiding the real failure.
💡 Fix: use assert inside the closure
require.NotPanics(t, func() {
result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
assert.NoError(t, err)
assert.NotEmpty(t, result)
}, "nil PinContext.Ctx should fall back to context.Background() without panicking")@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (104 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three threads in commit
|
|
@copilot please run the
|
… strength Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three
|
|
@copilot resolve the merge conflicts on this branch. |
…t-improve-test-quality-yet-again # Conflicts: # .github/workflows/agentic-auto-upgrade.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merge conflicts resolved. The only conflict was in |
PR Triage\n\n| Field | Value |\n|---|---|\n| Category | test |\n| Risk | low |\n| Score | 38/100 (impact:15, urgency:8, quality:15) |\n| Action | auto_merge |\n\nPure test quality improvements for actionpins spec_test.go. No production code changes. Auto-merge when CI passes.
|
Addresses seven test quality items from the Daily Testify Uber Super Expert audit of
pkg/actionpins/spec_test.go.Changes
New test:
TestSpec_DynamicResolution_EmptySHAFallsThrough— covers theresolveActionPinDynamicallybranch where the resolver returns("", nil); verifies fall-through to hardcoded pins for known repos and empty result for unknown repos.New test:
TestSpec_PublicAPI_ResolveActionPin_NilCtxField— verifies that a nilPinContext.Ctxfalls back tocontext.Background()viacmp.Orwithout panicking, and the resolver receives a non-nil context.Resolver-succeeds case in
TestSpec_PublicAPI_ResolveActionPin_EnforcePinned— the existing table only covered failure paths; adds the missing happy path where a dynamic resolver succeeds withEnforcePinned=trueand should return a pinned reference without error. AddedwantResultSHAfield to the table struct to distinguish this from the downgrade-to-warning case.@sha256:format assertions inTestSpec_PublicAPI_GetContainerPin— adds explicit prefix check onpin.Digestand@sha256:notation check onpin.PinnedImage.Merged
TestSpec_PublicAPI_RecordResolutionFailure*pair — the two standalone flat tests (no-resolverandfailing-resolvervariants) are merged into a single table-driven test, reducing duplication.Normalised assertion style in
TestSpec_DesignDecision_FormatConsistency— mixedassert.Truef/assert.Containsreplaced with consistentassert.Truef/assert.Containsfthroughout.Doc comment on
testSHAResolver.ResolveSHA— the method was the only unexported symbol in the file lacking a doc comment.