Improve actionpins internal test coverage for fallback and pin-resolution edge cases#42834
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey The PR is tightly scoped to a single test file, the description is thorough and well-structured, and no new dependencies are introduced. This looks ready for review. 🚀
|
There was a problem hiding this comment.
Pull request overview
This pull request expands and clarifies the internal unit test coverage for pkg/actionpins/actionpins.go, focusing on semver-compatible fallback selection, exact hardcoded pin resolution, warning deduplication behavior, and a few additional edge cases around pin-key mismatch counting.
Changes:
- Added new/expanded tests covering
findCompatiblePin,resolveNonStrictHardcodedPin, and additionalresolveExactHardcodedPinbranches (by version + no match). - Refactored MCP gateway container pin assertions into a single table-driven test.
- Expanded
countPinKeyMismatchestest coverage with subtests for empty input, all-matching input, invalid key format, and mixed entries; plus a small assertion cleanup (assert.Zero).
Show a summary per file
| File | Description |
|---|---|
| pkg/actionpins/actionpins_internal_test.go | Adds broader internal test coverage for action pin fallback, exact-match resolution, warning deduplication, container pinning, and mismatch counting edge cases. |
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: 0
- Review effort level: Low
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (170 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 Matter
ADRs 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
|
🧪 Test Quality Sentinel Report
📊 Metrics (21 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Improve actionpins internal test coverage
Good quality test addition overall — coverage of fallback paths, warning deduplication, and boundary expansion for countPinKeyMismatches all look correct. A few observations:
Correctness ✅
- All new assertions match the actual implementation logic in
actionpins.go. - Warning dedup test correctly captures that the
ctx.Warningsmap and stderr emission are both deduplicated on the samecacheKey. - The empty-version/no-match cases for
findCompatiblePinproperly exercise theIsCompatibleguard that returnsfalsefor invalid semver inputs. resolveExactHardcodedPintests correctly distinguish the SHA path (isAlreadySHA=true) from the version path (isAlreadySHA=false).
One non-blocking concern: misleading test case name
The "exact-version" case in TestFindCompatiblePin_SemverFallback (line 157–165) has a name that implies exact version matching, but findCompatiblePin performs major-compatible matching only. The test's custom availablePins (without v5.2.0) hides that requesting v5.0.0 when v5.2.0 is present would return v5.2.0, not v5.0.0. See inline comment for details. Non-blocking, but worth fixing to prevent reader confusion about the function's semantics.
Style ✅
- Table-driven consolidation of
TestGetContainerPin_MCPGatewayVersionsArePinnedis a clean improvement. assert.Zerousage is a good consistency improvement overassert.Equal(t, 0, ...).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 96.5 AIC · ⌖ 6.96 AIC · ⊞ 4.9K
| }, | ||
| { | ||
| name: "exact-version", | ||
| version: "v5.0.0", |
There was a problem hiding this comment.
The test case name "exact-version" is misleading: findCompatiblePin does not perform exact-version matching — it performs semver major-compatible matching (via semverutil.IsCompatible), returning the first pin whose major version matches the request.
The availablePins slice for this case was deliberately constructed without v5.2.0, so the result happens to be v5.0.0. But if the full pins slice (with v5.2.0 before v5.0.0) were used, findCompatiblePin(pins, "v5.0.0") would return v5.2.0, not v5.0.0, because that is the first major-compatible match.
This creates a false impression that the function honors exact version constraints. Consider:
- Renaming the case to something like
"first-major-compatible-wins"or"major-compatible-match". - Adding a sibling case that explicitly demonstrates that requesting
v5.0.0from a list containingv5.2.0returnsv5.2.0, clarifying the "first compatible, not exact" semantic.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. The case was renamed from "exact-version" to "major-compatible-match" with a clarifying comment explaining that major-compatible matching is used (not exact-version), and that the only v5.x in that subtest's list happens to be v5.0.0. A sibling "first-compatible-not-exact" case was also added using the full pins slice to demonstrate that requesting v5.0.0 returns v5.2.0.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 145.3 AIC · ⌖ 7.99 AIC · ⊞ 6.6K
Comment /matt to run again
| name: "exact-major", | ||
| version: "v5", | ||
| wantFound: true, | ||
| wantVersion: "v5.2.0", |
There was a problem hiding this comment.
[/tdd] The exact-major case implicitly depends on the pin list being sorted descending. findCompatiblePin returns the first compatible match, so if the list were [v5.0.0, v5.2.0], wantVersion would need to be "v5.0.0" — the hidden ordering assumption is unspecified in the test.
💡 Suggested improvement
Add a dedicated subtest that makes the first-match / pre-sorted semantics explicit:
{
name: "returns-first-compatible-not-highest",
version: "v5",
wantFound: true,
wantVersion: "v5.0.0", // first compatible, list unsorted
availablePins: []ActionPin{
{Version: "v5.0.0", SHA: "sha-low"},
{Version: "v5.2.0", SHA: "sha-high"},
},
},resolveNonStrictHardcodedPin relies on callers supplying a pre-sorted slice (via GetActionPinsByRepo). Making this contract visible in the test prevents a future contributor from reversing the sort and getting subtly wrong pins.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. Added "returns-first-compatible-not-highest" with an unsorted list [v5.0.0, v5.2.0], demonstrating that findCompatiblePin returns v5.0.0 (the first match) rather than v5.2.0. This makes the pre-sorted-slice contract explicit for callers.
| stderrOutput := testutil.CaptureStderr(t, func() { | ||
| result = resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx) | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/tdd] stderrOutput contains ANSI-styled text from console.FormatWarningMessage (the ⚠ prefix plus applyStyle). An assert.Contains on a plain-string expectedWarning may pass in CI but fail in environments where the style rendering differs (e.g., no-color vs. colour-capable terminals).
💡 Suggested improvement
Strip ANSI codes before asserting, or match only the unambiguous message substring:
// Option A — assert on a known stable fragment
assert.Contains(t, stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0")
// Option B — strip ANSI before comparing
import "github.com/acarl005/stripansi"
assert.Contains(t, stripansi.Strip(stderrOutput), expectedWarning)Alternatively, testutil.CaptureStderr could normalise ANSI internally and document that guarantee.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. All warning assertions now use the stable unstyled fragment "using hardcoded pin for actions/checkout@v5.2.0" instead of the full ANSI-decorated string, removing fragility across different terminal environments.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| pin, found := findCompatiblePin(tt.availablePins, tt.version) |
There was a problem hiding this comment.
[/tdd] The no-found branch asserts assert.Empty on both pin.Version and pin.SHA, but the returned ActionPin{} zero-value also has an empty Repo. Consider adding assert.Empty(t, pin.Repo) or a single assert.Equal(t, ActionPin{}, pin) to verify the full zero-value contract of the not-found return.
💡 Suggested fix
} else {
assert.Equal(t, ActionPin{}, pin, "expected zero-value ActionPin when not found")
}This is more robust than checking individual fields — if ActionPin gains new fields, the test still validates the whole zero-value.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. Replaced assert.Empty(t, pin.Version) + assert.Empty(t, pin.SHA) with assert.Equal(t, ActionPin{}, pin, "expected zero-value ActionPin when not found"), which validates the full zero-value contract including any future fields.
|
|
||
| result, ok := resolveExactHardcodedPin("actions/checkout", "v5.0.0", false, pins) | ||
|
|
||
| require.True(t, ok, "Expected exact version match to resolve") |
There was a problem hiding this comment.
[/tdd] TestResolveExactHardcodedPin_ByVersion calls resolveExactHardcodedPin with isAlreadySHA=false but there is no test for the case where the requested version matches both a pin version and is technically a valid SHA string (i.e., isAlreadySHA=true + a matching version). The exact-version branch and the exact-SHA branch inside resolveExactHardcodedPin are separate code paths — the new tests cover only one.
💡 Suggested addition
func TestResolveExactHardcodedPin_VersionTakesPrecedenceOverSHA(t *testing.T) {
// A version string that is also a syntactically valid SHA
pins := []ActionPin{{Repo: "actions/checkout", Version: "sha-v5", SHA: "sha-v5"}}
result, ok := resolveExactHardcodedPin("actions/checkout", "sha-v5", false, pins)
require.True(t, ok)
assert.Contains(t, result, "sha-v5")
}This is a boundary the existing coverage silently skips.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. Added TestResolveExactHardcodedPin_VersionTakesPrecedenceOverSHA using a pin where Version == SHA == "sha-v5" and isAlreadySHA=false. This shows the version-match loop runs and returns before the SHA loop is ever reached.
| } | ||
|
|
||
| t.Run("uses semver-compatible pin when available", func(t *testing.T) { | ||
| ctx := &PinContext{Warnings: make(map[string]bool)} |
There was a problem hiding this comment.
[/tdd] TestResolveNonStrictHardcodedPin_CompatibleAndFallback — the "uses semver-compatible pin" subtest calls resolveNonStrictHardcodedPin once and asserts that the warning is emitted. But it doesn't assert that a second call with the same version also deduplicates the warning (the dedup guard is at the resolveNonStrictHardcodedPin level, not just the fallback branch). That scenario is covered in the second subtest, but only for v9 (no-compatible path). The compatible path leaves a gap.
💡 Suggested addition
t.Run("deduplicates warning on compatible path", func(t *testing.T) {
ctx := &PinContext{Warnings: make(map[string]bool)}
stderrOutput := testutil.CaptureStderr(t, func() {
resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx)
resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx)
})
assert.Equal(t, 1, strings.Count(stderrOutput, "v5.2.0"),
"Expected warning emitted exactly once for repeated compatible resolution")
assert.Len(t, ctx.Warnings, 1)
})@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. Added a "deduplicates warning on compatible path" subtest in TestResolveNonStrictHardcodedPin_SelectsHighestCompatible that calls resolveNonStrictHardcodedPin twice with v5 and asserts the warning is emitted exactly once and ctx.Warnings has length 1.
| } | ||
| } | ||
|
|
||
| func TestFindCompatiblePin_SemverFallback(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestFindCompatiblePin_SemverFallback has no subtest for a minor-version constraint (e.g. v5.1 requested, list has v5.2.0 and v5.0.0). If semverutil.IsCompatible handles minor pinning differently from major pinning, that logic is invisible from the current test matrix.
💡 Suggested case
{
name: "exact-minor",
version: "v5.1",
wantFound: true,
wantVersion: "v5.2.0", // or v5.1.x if IsCompatible is minor-strict
availablePins: []ActionPin{
{Version: "v5.2.0", SHA: "sha-a"},
{Version: "v5.0.0", SHA: "sha-b"},
},
},Even if v5.1 is currently not used in practice, documenting the expected behaviour in a test makes the semantics of IsCompatible explicit for this package.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. Added "minor-version-constraint" case requesting v5.1 from [v5.2.0, v5.0.0], returning v5.2.0. This documents that IsCompatible performs major-only comparison, so minor constraints do not restrict the match.
| assert.Empty(t, result, "Expected empty result when no exact match is found") | ||
| } | ||
|
|
||
| func TestResolveNonStrictHardcodedPin_CompatibleAndFallback(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test name TestResolveNonStrictHardcodedPin_CompatibleAndFallback bundles two distinct behaviours (compatible selection and no-compatible fallback) into one test function. Per /tdd, test names should read as specifications. Splitting into TestResolveNonStrictHardcodedPin_SelectsHighestCompatible and TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible would make failures immediately diagnostic without reading subtests.
💡 Impact
When this test fails, the failing test name should tell you which behaviour broke. With bundled subtests under a single function, the failure shows TestResolveNonStrictHardcodedPin_CompatibleAndFallback/falls_back_to_highest... — which is reasonable but slightly noisier. The existing subtest names are good; the top-level function name is the gap.
This is a low-priority style note, not a blocking issue.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 9fd0ebe. TestResolveNonStrictHardcodedPin_CompatibleAndFallback was split into two separate top-level test functions: TestResolveNonStrictHardcodedPin_SelectsHighestCompatible and TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible. This is now outdated.
🤖 PR Triage — §28558548389
Score breakdown: Impact 18 · Urgency 10 · Quality 16 Rationale: Test-only PR adding internal coverage for
|
|
@copilot run pr-finisher skill |
- Rename 'exact-version' → 'major-compatible-match' and add
'first-compatible-not-exact' case clarifying first-match semantics (thread 1)
- Add 'returns-first-compatible-not-highest' case making list-ordering
contract explicit (thread 2)
- Assert on stable unstyled fragment to remove ANSI fragility (thread 3)
- Replace assert.Empty×2 with assert.Equal(t, ActionPin{}, pin) in
not-found branch (thread 4)
- Add TestResolveExactHardcodedPin_VersionTakesPrecedenceOverSHA (thread 5)
- Add 'deduplicates warning on compatible path' subtest (thread 6)
- Add 'minor-version-constraint' case for v5.1 major-only matching (thread 7)
- Split TestResolveNonStrictHardcodedPin_CompatibleAndFallback into
TestResolveNonStrictHardcodedPin_SelectsHighestCompatible and
TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible (thread 8)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All 8 unresolved review threads have been addressed in the two new commits:
|
|
@copilot please run the
|
All 8 unresolved review threads have been replied to (replies posted in this run). The code changes addressing each thread were already committed in
Local validation: Note: review threads could not be programmatically resolved (GraphQL API blocked in this environment) — a maintainer can resolve them manually or they will auto-resolve on merge. |
|
@copilot please run the
|
All 5 threads listed (and the remaining 3) already have code changes committed in |
🤖 PR Triage — Run §28572743533
|
|
@copilot please run the
|
Branch is already up to date with main (last merged at The only remaining step is that review threads cannot be resolved programmatically (GraphQL mutations are blocked in this environment). A maintainer needs to resolve them manually, then re-trigger CI. |
|
🎉 This pull request is included in a new release. Release: |
This updates
pkg/actionpins/actionpins_internal_test.goto cover previously untested resolution branches and edge cases inpkg/actionpins/actionpins.go, with emphasis on semver fallback behavior, exact-match resolution, and warning dedup semantics.findCompatiblePinsemver fallback coverageTestFindCompatiblePin_SemverFallbackfor:v5→ highest compatiblev5.x)Non-strict hardcoded fallback behavior
TestResolveNonStrictHardcodedPin_CompatibleAndFallbackcovering:Exact hardcoded resolution branches
TestResolveExactHardcodedPin_ByVersionTestResolveExactHardcodedPin_NoMatchContainer pin tests refactor
TestGetContainerPin_MCPGatewayVersionsArePinnedMismatch counting boundary expansion
TestCountPinKeyMismatches_ReturnsOnlyVersionMismatcheswith subtests for:@version)Assertion clarity cleanup
assert.Zeroin SHA-skip dynamic resolution test.