test(actionpins): improve test quality in actionpins_internal_test.go - #48707
Conversation
- Move countingResolver helper type near imports for readability - Convert TestGetContainerPin_DefaultMCPImagesArePinned to use t.Run per image - Add TestGetActionPins_CacheCorrectnessOnRepeatedCalls for cache validation - Use assert.PanicsWithValue in both panic tests to catch message regressions - Replace inner loop with assert.Equal in TestInitWarnings_InitializesAndPreservesMap - Add strict-mode exact-match subtest to StrictModeNoFallback test - Consolidate four TestResolveExactHardcodedPin_* into one table-driven test - Add comment banners for resolveExactHardcodedPin and resolveNonStrictHardcodedPin sections Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves actionpins unit-test organization, diagnostics, and branch coverage.
Changes:
- Adds stronger panic and strict-mode assertions.
- Introduces subtests and a cache-consistency test.
- Consolidates exact-pin tests into a table-driven suite.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/actionpins_internal_test.go |
Refactors and expands internal action-pin 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: 2
- Review effort level: Medium
| existing := map[string]bool{"actions/checkout@v5": true} | ||
| ctx := &PinContext{Warnings: existing} |
| { | ||
| // When isAlreadySHA=false, only the version-match path runs; the SHA-match loop is | ||
| // skipped entirely. This case uses a pin whose Version and SHA fields are identical | ||
| // to make the path selection explicit: the version loop matches and returns before | ||
| // the SHA loop would ever execute. | ||
| name: "version-match path takes precedence over SHA path when isAlreadySHA=false", | ||
| pins: []ActionPin{{Repo: "actions/checkout", Version: "sha-v5", SHA: "sha-v5"}}, | ||
| repo: "actions/checkout", | ||
| version: "sha-v5", | ||
| isSHA: false, | ||
| wantOK: true, | ||
| wantContains: []string{"sha-v5"}, | ||
| }, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
![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 actionpins
Good overall improvements — table-driven tests, subtests for better isolation, and tighter panic-value assertions. Two existing inline comments (lines 172 and 566) flag real issues; one new comment added at line 460.
Summary of findings:
- Line 172 (existing):
existingandctx.Warningspoint to the same map — an in-place mutation would make the equality assertion pass spuriously. Use a separate expected map. - Line 460 (new): strict-mode subtest couples to live embedded data via
GetLatestActionPinByRepo; use a synthetic fixture to keep it deterministic. - Line 566 (existing): the "version-match takes precedence" case is not actually distinguishable with identical Version/SHA fields and
isSHA=false— the SHA loop never runs, so the comment's claim cannot be verified.
No blocking issues; all are non-blocking suggestions to strengthen test correctness.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.2 AIC · ⌖ 5.31 AIC · ⊞ 5K
Comments that could not be inline-anchored
pkg/actionpins/actionpins_internal_test.go:460
Non-blocking: This subtest calls GetLatestActionPinByRepo("actions/checkout") against live embedded data. If that action is removed from the embedded pins the require.True skip will hide the subtest entirely. Consider using a synthetic ActionPin fixture instead, keeping the test self-contained and deterministic.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (11 tests)
✅ Key Strengths
Verdict
|
Design Decision Gate — ADR RequiredThis PR makes significant changes to core test logic (116 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on three test correctness issues.
📋 Key Themes & Highlights
Issues Found
- Map aliasing (line 177):
existingandctx.Warningspoint to the same map; mutations to one silently affect the other, making the assertion vacuous. - Cache isolation not tested (line 434):
TestGetActionPins_CacheCorrectnessOnRepeatedCallsonly checks value equality, not that the cache returns independent copies; a mutable-reference regression would pass. - Ambiguous precedence test (line 566): The "version takes precedence" table row uses equal
VersionandSHAvalues, so both paths produce the same output and the test cannot distinguish a regression.
Positive Highlights
- ✅ Converting the
forloop tot.Runsubtests inTestGetContainerPin_DefaultMCPImagesArePinnedis a clear improvement — failure isolation is now correct. - ✅
assert.PanicsWithValueupgrades are well-targeted and will catch silent message drift. - ✅ Consolidating four
TestResolveExactHardcodedPin_*functions into one table test improves readability and maintainability significantly. - ✅ Moving
countingResolverto the top of the file makes the helper's scope immediately clear.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.5 AIC · ⌖ 4.99 AIC · ⊞ 6.7K
Comment /matt to run again
| r.called++ | ||
| return "", nil | ||
| require.NotEmpty(t, first, "Expected at least one action pin in embedded data") | ||
| assert.Equal(t, first, second, "Expected repeated calls to getActionPins() to return equal data (cache correctness)") |
There was a problem hiding this comment.
[/tdd] assert.Equal(first, second) confirms the returned values are equal but does not verify cache isolation — if getActionPins() returns the same backing slice, a mutation to first silently corrupts second and the test still passes.
💡 Suggested addition
Add a mutation-isolation check:
if len(first) > 0 {
original := first[0]
first[0].SHA = "tampered"
second2 := getActionPins()
assert.Equal(t, original.SHA, second2[0].SHA,
"cache must not expose a mutable reference")
first[0] = original
}Alternatively, document that callers must treat the returned slice as read-only.
@copilot please address this.
| for warning := range existing { | ||
| assert.True(t, ctx.Warnings[warning], "Expected warning %q to be preserved", warning) | ||
| } | ||
| assert.Equal(t, existing, ctx.Warnings, "Expected existing warnings to be preserved unchanged") |
There was a problem hiding this comment.
[/tdd] existing and ctx.Warnings now reference the same map, so any in-place mutation (e.g. initWarnings clearing or replacing the map) would also update existing, making the assertion trivially true and hiding regressions.
💡 Suggested fix
Pass a copy so the expected and actual values are independent:
existing := map[string]bool{"actions/checkout@v5": true}
ctx := &PinContext{Warnings: map[string]bool{"actions/checkout@v5": true}}
initWarnings(ctx)
require.NotNil(t, ctx.Warnings)
assert.Equal(t, existing, ctx.Warnings)@copilot please address this.
| isSHA: false, | ||
| wantOK: true, | ||
| wantContains: []string{"sha-v5"}, | ||
| }, |
There was a problem hiding this comment.
[/tdd] The "version-match path takes precedence" case uses Version: "sha-v5", SHA: "sha-v5" with isSHA: false, but since both fields are identical, a regression that accidentally runs the SHA-match path would still produce the same result and go undetected.
💡 Suggested fix
Use distinct values for Version and SHA so the two paths are distinguishable:
{
name: "version-match path takes precedence over SHA path when isAlreadySHA=false",
pins: []ActionPin{{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"}},
repo: "actions/checkout",
version: "v5.0.0",
isSHA: false,
wantOK: true,
wantContains: []string{"sha-v5", "v5.0.0"},
},This ensures the test fails if the SHA-match path runs instead of the version-match path.
@copilot please address this.
|
@copilot run pr-finisher skill |
- TestInitWarnings: build expected map independently of ctx.Warnings to prevent aliased-map false passes on in-place mutations - TestResolveExactHardcodedPin: replace isSHA=false precedence row with isSHA=true and two distinct pins so the test can distinguish which code path (version vs SHA loop) was actually taken Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both reviewer issues in the latest commit:
|
|
🎉 This pull request is included in a new release. Release: |
Addresses several test quality gaps in
pkg/actionpins/actionpins_internal_test.go: missing subtests, weak panic assertions, duplicated test functions, and a misplaced helper type.Changes
countingResolverrelocated — moved from mid-file to immediately after imports with a clarifying commentt.Runper image inTestGetContainerPin_DefaultMCPImagesArePinned— plainforloop converted to subtests, matching the pattern inTestGetContainerPin_MCPGatewayVersionsArePinned; a failure on one image no longer aborts the restCache-correctness test — new
TestGetActionPins_CacheCorrectnessOnRepeatedCallsdirectly asserts that repeatedgetActionPins()calls return equal dataassert.PanicsWithValuein panic tests — bothTestFormatPinnedActionReference_PanicsWhenSHAIsEmptyandTestLoadActionPinsData_PanicsWhenEntrySHAIsEmptynow assert the exact panic string, catching silent message changes:Strict-mode exact-match subtest —
TestResolveActionPinFromHardcodedPins_StrictModeNoFallbackgains a second subtest verifying an exact version match still resolves whenStrictMode: true, covering the previously untested branchTable-driven
TestResolveExactHardcodedPin— four separateTestResolveExactHardcodedPin_*top-level functions (BySHA, ByVersion, NoMatch, VersionTakesPrecedenceOverSHA) consolidated into onename/pins/repo/version/isSHA/wantOK/wantContainstable, consistent with the style used elsewhere in the fileassert.Equalreplaces per-key loop inTestInitWarnings_InitializesAndPreservesMap's "preserves existing" subtest for a single clear diff on failureComment banners added for
resolveExactHardcodedPinandresolveNonStrictHardcodedPinsections for visual grouping