Tighten pkg/actionpins spec coverage around formatter edge cases and mapping behavior#47379
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/actionpins spec coverage around formatter edge cases and mapping behavior
There was a problem hiding this comment.
Pull request overview
Strengthens pkg/actionpins specification tests without changing production behavior.
Changes:
- Covers formatter empty-version and empty-SHA behavior.
- Tests self-mapping resolution and notification recording.
- Improves assertion gating and test organization.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/spec_test.go |
Expands edge-case coverage and improves test failure isolation. |
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: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (57 additions, threshold 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![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 ✅
The changes are correct and well-targeted. Quick verification on key points:
- Panic message matches the production string in
actionpins.goline 260 exactly. - Warning key
"map:actions/checkout@v4"is the correct output ofFormatCacheKeyfor the self-mapping test. - Empty version comment test expectation
"actions/checkout@abc123 # "is consistent with therepo@sha # versionformat contract. require.NoErrorinsiderequire.NotPanics— valid improvement;t.FailNow()is safe to call from within aNotPanicsclosure in Go test goroutines.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 22.2 AIC · ⌖ 4.51 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — changes are test-only, well-structured, and clearly aligned with the spec.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Panic test correctly uses
require.PanicsWithValuewith the exact message string - ✅
require.NoErrorupgrades insiderequire.NotPanicsclosures are the right call — they stop execution on bad state - ✅ Self-mapping test properly validates both the resolved value and the warning key — good dual-assertion discipline
- ✅ Wrapping
TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatchin a namedt.Runimproves consistency and failure readability - ✅ Empty-version-comment case (
"actions/checkout@abc123 # ") is a useful edge case that pins the trailing-space behaviour explicitly
One Issue Found
A single inline comment was left: the wantPanic/expected fields in the table struct are mutually exclusive but not enforced. A stale copy-paste row could silently skip assertions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 22 AIC · ⌖ 4.66 AIC · ⊞ 6.7K
Comment /matt to run again
| }, | ||
| { | ||
| name: "panics for empty sha", | ||
| repo: "actions/checkout", |
There was a problem hiding this comment.
[/tdd] wantPanic and expected are mutually exclusive fields, but the table loop doesn't enforce that — a row with both set will silently skip the expected assertion.
💡 Suggested guard
Add a quick sanity check at the top of the panic branch:
if tt.wantPanic != "" {
assert.Empty(t, tt.expected, "test case %q: wantPanic and expected are mutually exclusive", tt.name)
require.PanicsWithValue(t, tt.wantPanic, func() {
actionpins.FormatPinnedActionReference(tt.repo, tt.sha, tt.version)
})
return
}This makes the mutual-exclusion contract explicit and catches copy-paste mistakes early.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 issues must be fixed before merging
The test additions cover real gaps but have three concrete problems.
### Blocking issues
-
Non-hermetic self-mapping test (high): The new sub-test calls
ResolveActionPintwice against the real GitHub API. This will fail in air-gapped CI and on rate-limit. Use thetestSHAResolverstub already in the file. See inline comment at line 106. -
Trailing-space expected value (high):
"actions/checkout@abc123 # "canonicalizes a dangling comment marker. If this is intentional production behavior, it needs a comment explaining why; if it is a bug in the implementation, fix it rather than ossifying it in a test. See inline comment at line 33. -
Warning assertion lacks negative baseline (medium): The
assert.True(t, ctx.Warnings["map:actions/checkout@v4"])assertion has no corresponding negative check on the no-mapping baseline, so it cannot distinguish a mapping-triggered warning from one that fires unconditionally. See inline comment at line 109.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 32.3 AIC · ⌖ 4.92 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/actionpins/spec_test.go:106
Self-mapping sub-test makes real network calls: both baseline and result calls hit the live GitHub API with no stub, making this test non-hermetic and CI-fragile.
<details>
<summary>💡 Suggested fix</summary>
Use the testSHAResolver already present in this file and inject it via PinContext:
resolver := &testSHAResolver{shas: map[string]string{"actions/checkout@v4": "abc123"}}
ctx := &actionpins.PinContext{
Warnings: make(map[string]bool),
SHAResolver: resolver,
…
</details>
<details><summary>pkg/actionpins/spec_test.go:33</summary>
**Expected value has a trailing space after `# `**: the test asserts `"actions/checkout@abc123 # "` — a dangling comment marker with whitespace — which may be documenting a bug rather than valid behavior.
<details>
<summary>💡 Details</summary>
When `version` is empty, the most defensible outputs are either:
- Omit the comment entirely: `"actions/checkout@abc123"`
- Or emit the marker with a blank: `"actions/checkout@abc123 # "`
If the implementation intentionally emits the trailing space, t…
</details>
<details><summary>pkg/actionpins/spec_test.go:109</summary>
**Self-mapping warning assertion tests unverified behavior**: the test assumes that an identity mapping (`A→A`) records a warning key, but there is no baseline check showing the key is absent without a mapping.
<details>
<summary>💡 Details</summary>
The assertion `assert.True(t, ctx.Warnings["map:actions/checkout@v4"], ...)` is only meaningful if:
1. The implementation actually emits this key for self-mappings, and
2. The no-mapping baseline does **not** emit it (otherwise the mapping is i…
</details>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (3 tests)
Notes
Verdict
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
pkg/actionpins/spec_test.gowas missing coverage for a few high-value edge cases and had a couple of assertion/organization gaps that reduced failure signal. This update strengthens the spec tests without changing production behavior.Formatter edge cases
FormatPinnedActionReferencewith:Mapping behavior
TestSpec_PublicAPI_ResolveActionPin_AppliesMappingrepo@ref -> same repo@refpreserves the resolved output while still recording the mapping notification keyAssertion upgrades
assert.NoErrorwithrequire.NoErrorinside the tworequire.NotPanicsclosuresTest organization
TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatchin a namedt.Runfor consistency with the rest of the file{ name: "formats reference with empty version comment", repo: "actions/checkout", sha: "abc123", version: "", expected: "actions/checkout@abc123 # ", }, { name: "panics for empty sha", repo: "actions/checkout", sha: "", version: "v4", wantPanic: "FormatPinnedActionReference called with empty SHA for repo=actions/checkout version=v4 — this would produce invalid workflow YAML", }