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

[github-actions]

Overview

File: pkg/actionpins/spec_test.go
Paired source: pkg/actionpins/actionpins.go
Test functions: 31 top-level Test* functions, many with subtests via t.Run
LOC: 988

Strengths

• Excellent table-driven patterns for FormatPinnedActionReference, ExtractRepo, ExtractVersion.
• Good require/assert discipline overall — blocking failures use require before deriving further checks.
• Regression tests with inline comments explaining the why (e.g. non-deterministic pin comments bug).
• Concurrency test (TestSpec_ThreadSafety_ConcurrentGetActionPinsByRepo) using sync.WaitGroup.
• Context-propagation tests verify both nil-ctx fallback and cancellation state forwarding.

---

Prioritized Improvements

1. Missing / high-value test cases

FormatCacheKey — only 2 table rows; missing edge cases

TestSpec_PublicAPI_FormatCacheKey (line 100) only covers two happy-path inputs. The function is called throughout the codebase as a deduplication key. Missing cases:

Input	Why it matters
("", "")	Dedup key for empty inputs shouldn't panic
("repo", "")	Empty version should still produce "repo@"
("", "v4")	Empty repo edge

Before:

{
    name:     "formats cache key as repo@version",
    repo:     "actions/checkout",
    version:  "v4",
    expected: "actions/checkout@v4",
},
{
    name:     "formats cache key with full semver",
    repo:     "actions/setup-node",
    version:  "v3.0.0",
    expected: "actions/setup-node@v3.0.0",
},

After (add these rows):

{
    name:     "empty repo and version still formats separator",
    repo:     "",
    version:  "",
    expected: "@",
},
{
    name:     "empty version formats trailing @",
    repo:     "actions/checkout",
    version:  "",
    expected: "actions/checkout@",
},

ResolveLatestActionPin_FallbackOnEnforceError — unknown-repo subtest doesn't verify failure recording

The second subtest (line 470) uses a PinContext with EnforcePinned: true but no RecordResolutionFailure callback. If a failure is supposed to be recorded, there is no assertion to verify it. Consider adding a callback and asserting it is (or isn't) called:

t.Run("unknown repo returns empty when no embedded fallback exists", func(t *testing.T) {
    var failures []actionpins.ResolutionFailure
    ctx := &actionpins.PinContext{
        EnforcePinned: true,
        Warnings:      make(map[string]bool),
        RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
            failures = append(failures, f)
        },
    }
    result := actionpins.ResolveLatestActionPin("does-not-exist/x", ctx)
    assert.Empty(t, result, "unknown repo should return empty when no embedded fallback exists")
    // Assert that a failure was or wasn't recorded based on the spec contract.
    assert.NotEmpty(t, failures, "resolution failure should be recorded for unknown repo")
})

No test for ApplyContainerPinMapping with an invalid SHA length (63 or 65 hex chars)

The function rejects mappings without a valid @sha256:<64-hex> digest. There is a test for "no digest at all" (line 977) but not for a malformed digest (wrong length, non-hex chars). Both rejection cases are distinct code paths that should be covered.

t.Run("invalid mapping - sha256 digest too short returns image unchanged", func(t *testing.T) {
    ctx := &actionpins.PinContext{
        Warnings: make(map[string]bool),
        ContainerMappings: map[string]string{
            "ghcr.io/owner/image:latest": "registry.acme.com/image:latest@sha256:tooshort",
        },
    }
    result := actionpins.ApplyContainerPinMapping("ghcr.io/owner/image:latest", ctx)
    assert.Equal(t, "ghcr.io/owner/image:latest", result,
        "malformed digest should be rejected and image returned unchanged")
})

2. Testify assertion upgrades

Missing assertion message on assert.Equal in table loop (line 364)

Inside TestSpec_PublicAPI_ResolveActionPin_EnforcePinned, the failure-type assertion has no message:

// Before (line 364) — no message
assert.Equal(t, tt.wantFailureType, failures[0].ErrorType)

// After — add message for easier diagnosis
assert.Equal(t, tt.wantFailureType, failures[0].ErrorType,
    "failure ErrorType should match expected for case %q", tt.name)

This is the only assert.Equal in the file without a message, making failures harder to diagnose.

Use require.Equal instead of assert.Equal for final equality checks that produce no further useful output on failure

In TestSpec_PublicAPI_ResolveActionPin_NilContext (lines 270-273) and TestSpec_PublicAPI_ResolveActionPin_UnknownFullSHAReturnsFormattedReference (lines 283-287), the test has require.NoError followed by assert.Equal on result. Since result is only interesting when there's no error (already guarded by require), assert is acceptable here — but consider whether you want the test to stop immediately on the equality failure rather than continue to unrelated subtests. Both patterns are fine; the file is inconsistent (some use require.Equal, some use assert.Equal for the same kind of structural assertion).

At minimum, standardise: use assert.Equal for value checks that are the terminal assertion in a short test, and require.Equal only when subsequent code depends on the asserted value.

3. Table-driven refactors

TestSpec_PublicAPI_RecordResolutionFailure duplicates PinContext construction

The two table cases in TestSpec_PublicAPI_RecordResolutionFailure (lines 633-677) each construct an identical PinContext inside t.Run, differing only by Resolver. The failure-collection slice is also duplicated. Extract the setup:

for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        var failures []actionpins.ResolutionFailure
        ctx := &actionpins.PinContext{
            Resolver: tt.resolver,
            Warnings: make(map[string]bool),
            RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
                failures = append(failures, f)
            },
        }
        _, err := actionpins.ResolveActionPin(tt.repo, tt.version, ctx)
        require.NoError(t, err)
        require.Len(t, failures, 1)
        assert.Equal(t, tt.wantErrorType, failures[0].ErrorType,
            "failure should be classified with the expected error type")
        assert.Equal(t, tt.repo, failures[0].Repo)
        assert.Equal(t, tt.version, failures[0].Ref)
    })
}

This is already the structure — it's good. No changes needed here.

4. Organisation / readability

testSHAResolver is defined in spec_test.go but may be reusable across test files

testSHAResolver (lines 27-41) is defined in spec_test.go (package actionpins_test). If actionpins_internal_test.go (package actionpins) needs similar fake resolvers, they are duplicated. Consider moving shared test helpers to a testhelpers_test.go file in the same package if duplication appears.

Currently no duplication exists, but this is worth tracking as the test suite grows.

---

Acceptance Checklist

• Add edge-case rows to TestSpec_PublicAPI_FormatCacheKey table (empty repo, empty version, both empty)
• Add RecordResolutionFailure callback + assertion to the unknown-repo subtest in TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError
• Add malformed-digest subtest to TestSpec_PublicAPI_ApplyContainerPinMapping
• Add message string to the un-messaged assert.Equal on line 364
• Run make test-unit and confirm all tests pass

Generated by 🧪 Daily Testify Uber Super Expert · sonnet46 · 55.2 AIC · ⌖ 11.8 AIC · ⊞ 5.2K · ◷

• expires on Jul 29, 2026, 10:32 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 · gpt54 · 3.26 AIC · ⌖ 7.77 AIC · ⊞ 8.3K · ◷