[actionpins] Add missing internal coverage and table-drive action pin mapping tests - #47206
Conversation
…ng tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Pure test improvement — adds missing internal coverage and consolidates table-driven tests in
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds targeted internal test coverage and simplifies action-pin mapping tests.
Changes:
- Covers container loading, latest-pin formatting, skipped resolution, and failure recording.
- Consolidates mapping scenarios into a table-driven test.
- Documents the black-box specification test package.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/actionpins_internal_test.go |
Adds coverage and consolidates mapping tests. |
pkg/actionpins/spec_test.go |
Adds the package documentation comment. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are well-structured: new tests cover previously untested paths (loadActionPinsData container field, getLatestActionPinReference, logDynamicResolutionSkipped, recordPinResolutionFailure nil safety), and the applyActionPinMapping consolidation improves maintainability without losing any scenario. The require.NotEmpty guard in TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmpty makes the embedded-data dependency explicit and safe. No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 11.1 AIC · ⌖ 7.91 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting with minor improvement suggestions; no blocking issues.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Table-driven consolidation of
TestApplyActionPinMapping_*is clean and well-structured — six cases, clear field names, all prior scenarios preserved - ✅
TestRecordPinResolutionFailure_NilSafetycovers three distinct nil-safety scenarios with correct use of sub-tests - ✅
TestLoadActionPinsData_LoadsContainerPinsuses a self-contained inline fixture — no external dependency - ✅ Package-level doc comment on
spec_test.goadds useful orientation for new contributors
Minor Issues
pins[0]coupling inTestGetLatestActionPinReference— assumes sort order without documenting the contract; a future re-ordering silently breaks the assertionTestLogDynamicResolutionSkipped_NoResolverBranchis incomplete — only tests the no-resolver branch; the resolver-present path is uncoveredrepeat: 0zero-value ambiguity — defaults to 1 viamax(tt.repeat, 1), but0reads as "run zero times", which could confuse future contributors
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 33.9 AIC · ⌖ 4.87 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| result := getLatestActionPinReference("actions/checkout") | ||
| assert.Equal(t, FormatPinnedActionReference("actions/checkout", pins[0].SHA, pins[0].Version), result) | ||
| }) |
There was a problem hiding this comment.
[/tdd] pins[0] coupling may assert the wrong value — getLatestActionPinReference implies latest, but the test assumes pins[0] is the latest without verifying sort order.
💡 Suggestion
If GetActionPinsByRepo guarantees a stable order (e.g., newest-first), document that assumption here. Otherwise, assert properties of the result rather than coupling to index 0.
// assert it is a non-empty, well-formed reference string instead of pinning to pins[0]
assert.NotEmpty(t, result)
assert.Contains(t, result, "actions/checkout@")A future re-ordering of embedded pins would silently produce a wrong assertion.
@copilot please address this.
| } | ||
|
|
||
| func TestLogDynamicResolutionSkipped_NoResolverBranch(t *testing.T) { | ||
| assert.NotPanics(t, func() { |
There was a problem hiding this comment.
[/tdd] TestLogDynamicResolutionSkipped_NoResolverBranch only tests (false, false) — the function has a second branch (the resolver-present path) that is untested here.
💡 Suggestion
Add a sub-test for the resolver-present branch to ensure both code paths are exercised:
t.Run("no resolver — does not panic", func(t *testing.T) {
assert.NotPanics(t, func() { logDynamicResolutionSkipped(false, false) })
})
t.Run("with resolver — does not panic", func(t *testing.T) {
assert.NotPanics(t, func() { logDynamicResolutionSkipped(true, false) })
})Without this, the resolver-present branch remains uncovered by the new test.
@copilot please address this.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx := &PinContext{ | ||
| Warnings: make(map[string]bool), |
There was a problem hiding this comment.
[/tdd] The table-driven TestApplyActionPinMapping uses repeat only for the deduplication case but silently defaults to 1 via max(tt.repeat, 1) for all others — a zero repeat in a struct literal is easy to misread as "run 0 times".
💡 Suggestion
Consider making the intent explicit with a named constant or by changing the zero-value semantics:
// Option A: default clearly in the test harness comment
repeat := tt.repeat
if repeat == 0 {
repeat = 1 // zero means "run once"
}
// Option B: use a pointer or a named field like runTimesThis is a minor readability issue, but it can confuse contributors adding future cases who might set repeat: 0 expecting no calls.
@copilot please address this.
… in actionpins Draft ADR documenting the test consolidation and coverage gap decisions in pkg/actionpins introduced by PR #47206. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (182 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✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (27 tests)
Verdict
|
There was a problem hiding this comment.
Two test correctness issues need fixing before merge
The consolidation to a table-driven test is a good call overall, but introduces two issues that undermine the test's effectiveness:
-
Line 706 — false/absent ambiguity (high):
ctx.Warnings[notifyKey]returns Go's zero value (false) for missing keys, soassert.Equal(t, false, ...)cannot distinguish "key never written" from "key set to false". ThewantMappingNotification: falsecases pass vacuously and would still pass if the production code accidentally wrote the key or if the internal key prefix silently changed. -
Line 699 — repeat loop hides first-iteration bugs (medium):
gotRepo/gotVersionare overwritten each iteration; only the last call's output is asserted. A regression where the first mapping application returns wrong values is completely invisible.
Both are straightforward to fix (see inline comments). No other blocking issues.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 46.3 AIC · ⌖ 4.76 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| repo, version := applyActionPinMapping("actions/checkout", "v4", ctx) | ||
| notifyKey := "map:" + FormatCacheKey(tt.actionRepo, tt.version) | ||
| assert.Equal(t, tt.wantMappingNotification, ctx.Warnings[notifyKey], "mapping notification flag should match expected state") |
There was a problem hiding this comment.
Absent key and explicit false are indistinguishable: assert.Equal(t, false, ctx.Warnings[notifyKey]) passes whether the key was never written or was set to false — Go returns the zero value for missing map keys. A wantMappingNotification: false test case will pass even if the production code accidentally wrote the key, hiding the bug silently.
💡 Suggested fix
Replace the single assertion with presence/absence checks:
if tt.wantMappingNotification {
assert.True(t, ctx.Warnings[notifyKey], "expected mapping notification key to be set")
} else {
assert.NotContains(t, ctx.Warnings, notifyKey, "expected mapping notification key to be absent")
}There is a second latent risk here: notifyKey is constructed by mirroring the production internal prefix ("map:"). If that prefix ever changes in applyActionPinMapping, the test will still pass — the checked key is absent, zero-value is false, which matches wantMappingNotification: false — masking the regression instead of catching it.
| } | ||
| var gotRepo, gotVersion string | ||
| for range repeat { | ||
| gotRepo, gotVersion = applyActionPinMapping(tt.actionRepo, tt.version, ctx) |
There was a problem hiding this comment.
Deduplication test only validates the final iteration's return values: gotRepo and gotVersion are overwritten on every loop pass; only the last call is ever asserted. For the repeat: 2 deduplication case, a bug where the first call returns wrong values (e.g., un-mapped repo/version on the first application) is completely invisible.
💡 Suggested fix
Assert inside the loop, or capture per-iteration results separately:
for i := range repeat {
repo, version := applyActionPinMapping(tt.actionRepo, tt.version, ctx)
assert.Equalf(t, tt.wantRepo, repo, "iteration %d: repo mismatch", i)
assert.Equalf(t, tt.wantVersion, version, "iteration %d: version mismatch", i)
}This makes each iteration's output observable and will catch regressions where the idempotency of repeated calls breaks.
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
This issue asked for targeted quality improvements in
pkg/actionpins/actionpins_internal_test.goplus a small readability tweak inspec_test.go. The suite was already strong; this change closes the remaining high-value gaps and consolidates duplicated mapping tests.Missing internal coverage added
TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmptyTestRecordPinResolutionFailure_NilSafetyTestLogDynamicResolutionSkipped_NoResolverBranchTestLoadActionPinsData_LoadsContainerPinsapplyActionPinMappingtests consolidatedTestApplyActionPinMapping_*functions with one table-drivenTestApplyActionPinMapping.@refin mapping target.Spec-test package docs
pkg/actionpins/spec_test.gofor clearer test-module intent.