[testify-expert] Improve Test Quality: pkg/actionpins/spec_test.go

[github-actions]

Overview

File: pkg/actionpins/spec_test.go
Source pair: pkg/actionpins/actionpins.go
Test functions: 28 top-level Test* functions (many with subtests)
LOC: 778 (test) / 514 (source)
Testify imports: both assert and require present

---

Strengths

• Good use of table-driven tests for pure-function cases (ExtractRepo, ExtractVersion, FormatPinnedActionReference, FormatCacheKey).
• Context propagation test (TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext) is excellent — verifies exact pointer identity with assert.Same.
• Concurrent safety test uses goroutines + sync.WaitGroup correctly.
• testSHAResolver helper is clean and captures call arguments for assertion.

---

Prioritized Improvements

1. Missing / high-value tests

Details

These exported functions/paths in actionpins.go have no test coverage in spec_test.go:

Function / Path	Risk
formatPinnedActionWithResolution (internal, but exercised via ResolveActionPin) — the two-version comment format (v4 → v4.1.2) is tested indirectly but never in a dedicated case	Medium
applyActionPinMapping with a mapped version that exists in embedded pins but the original does not — no negative mapping miss test	Medium
GetContainerPin — only 2 subtests; no test for a container whose PinnedImage does not contain its Digest (format contract)	Low
ResolveActionPin with ctx.Ctx == nil (zero-value PinContext) — resolver is called with context.Background() fallback path	Low

Highest-value addition — applyActionPinMapping negative path:

// Before (no test):
// applyActionPinMapping falls through silently when mapping target is unparseable.

// After:
func TestSpec_PublicAPI_ResolveActionPin_MappingTargetUnknown(t *testing.T) {
    ctx := &actionpins.PinContext{
        Warnings: make(map[string]bool),
        Mappings: map[string]string{
            "actions/setup-node@v4": "does-not-exist/missing@v99",
        },
    }
    result, err := actionpins.ResolveActionPin("actions/setup-node", "v4", ctx)
    require.NoError(t, err)
    assert.Empty(t, result, "mapping to an unknown repo should yield an empty pin, not a panic")
}

2. Testify assertion upgrades

Details

Several assert.* calls should be require.* because a failure makes the subsequent assertions meaningless:

Location	Current	Should Be	Reason
TestSpec_PublicAPI_ResolveActionPin_EnforcePinned L310	require.Len(t, failures, ...)	already require ✓	—
TestSpec_PublicAPI_RecordResolutionFailure_WarningDedup L710	require.Len(t, failures, 1, ...)	already require ✓	—
TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatch L470	require.NotEmpty(t, result, ...)	already require ✓	—
TestSpec_PublicAPI_GetContainerPin L547	require.NotEmpty(t, pin.Digest, ...)	already require ✓	—
TestSpec_PublicAPI_ResolveActionPin_AppliesMapping L727	require.NotEmpty(t, checkoutPins, ...)	already require ✓	—
TestBuildByRepoIndex_GroupsByRepoAndSortsDescending (internal test, L27-28)	assert.Equal for index 0 and 1	require for index assertions after require.Len	If Len passes but the slice is nil, index access panics

The internal test file actionpins_internal_test.go uses assert.Equal to index into slices after require.Len. This is safe only because require.Len halts the test on failure, but the index assertions should also be require to be explicit:

// Before (actionpins_internal_test.go ~L27):
assert.Equal(t, "v5.0.0", byRepo["actions/checkout"][0].Version, ...)
assert.Equal(t, "v4.0.0", byRepo["actions/checkout"][1].Version, ...)

// After:
require.Equal(t, "v5.0.0", byRepo["actions/checkout"][0].Version, ...)
require.Equal(t, "v4.0.0", byRepo["actions/checkout"][1].Version, ...)

This prevents a confusing index-out-of-bounds panic if the sort order invariant breaks.

3. Table-driven refactors

Details

TestSpec_PublicAPI_ResolveActionPin_EnforcePinned already uses a table — good. However, the four TestSpec_PublicAPI_ResolveLatestActionPin* tests share identical setup (fetching latestPin for actions/checkout) but are split into separate top-level functions. Consolidating into one table-driven test reduces boilerplate and setup duplication:

// Before: two separate top-level tests each calling GetLatestActionPinByRepo independently.

// After (sketch):
func TestSpec_PublicAPI_ResolveLatestActionPin(t *testing.T) {
    latestPin, ok := actionpins.GetLatestActionPinByRepo("actions/checkout")
    require.True(t, ok)

    tests := []struct{ name string; repo string; ctx *actionpins.PinContext; want string }{
        { name: "known repo nil ctx",   repo: "actions/checkout", ctx: nil, want: actionpins.FormatPinnedActionReference(...) },
        { name: "unknown repo nil ctx", repo: "does-not-exist/x", ctx: nil, want: "" },
        { name: "known repo non-nil ctx", ... },
    }
    for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... }) }
}

4. Organization / readability

Details

• SPEC_MISMATCH comments on lines 184 and 217 document real implementation divergences from the spec. These should either be filed as separate issues or resolved — leaving them as inline comments risks them going stale and misleading future readers.
• testSHAResolver placement: the helper type (L474-488) is declared in the middle of the file, between two test functions. Move it to the top of the file (after the const block) or to a helpers_test.go file so it is easy to discover.
• Test function naming: TestSpec_PublicAPI_* is verbose. The Spec_PublicAPI_ prefix is redundant when the package is already actionpins_test — consider TestResolveActionPin_NilContext instead of TestSpec_PublicAPI_ResolveActionPin_NilContext.

---

Acceptance Checklist

• Add TestSpec_PublicAPI_ResolveActionPin_MappingTargetUnknown (mapping to unknown repo yields empty result, no panic).
• Add test for applyActionPinMapping with a target that lacks @ separator.
• Upgrade slice-index assert.Equal calls in actionpins_internal_test.go to require.Equal after require.Len.
• Move testSHAResolver declaration to the top of spec_test.go (after const block).
• Resolve or file tracking issues for SPEC_MISMATCH comments (lines 184, 217).
• Run make test-unit and confirm all tests pass with no regressions.

Generated by 🧪 Daily Testify Uber Super Expert · 40 AIC · ⌖ 12.9 AIC · ⊞ 5.1K · ◷

• expires on Jul 15, 2026, 10:34 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · 1.48 AIC · ⌖ 5.91 AIC · ⊞ 6.2K · ◷