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

[github-actions]

Current State

• File: pkg/actionpins/actionpins_internal_test.go (869 lines, 27 top-level Test* funcs, many with t.Run subtests)
• Source pair: pkg/actionpins/actionpins.go (595 lines, 27 functions)
• Uses github.com/stretchr/testify/{assert,require} (66 assert.*, 25 require.* calls) and testutil.CaptureStderr for stderr assertions.

Strengths

• Good require vs assert discipline: require used for prerequisite/fatal checks (e.g. require.NotEmpty, require.True), assert for value checks.
• Strong table-driven coverage for TestFormatPinnedActionWithResolution_ConsistentVersionComment, TestFindCompatiblePin_SemverFallback, TestApplyActionPinMapping, TestApplyContainerPinMapping.
• Descriptive failure messages passed to nearly every assertion.
• Nil-safety and dedup-notification behavior explicitly tested (TestRecordPinResolutionFailure_NilSafety, dedup subtests in mapping tests).

Prioritized Improvements

1. Missing/high-value tests

• TestGetContainerPin_DefaultMCPImagesArePinned (lines 403–416) iterates over images with a plain for loop instead of t.Run(image, ...). A failure on one image aborts remaining assertions and reports poorly. Convert to subtests, matching the pattern already used in TestGetContainerPin_MCPGatewayVersionsArePinned.
• No test exercises getActionPins() caching behavior itself (singleton/lazy-load via sync.Once or similar) — only its side effect (cachedContainerPins) is probed indirectly in TestGetContainerPin_MCPGatewayVersionsArePinned. Add a direct test asserting repeated calls to getActionPins() return equal/identical data (cache correctness), e.g. assert.Same/assert.Equal on two invocations.
• TestResolveActionPinFromHardcodedPins_StrictModeNoFallback has only one scenario. Add a companion case where StrictMode: true with an exact version match still resolves (ok == true), so the "no fallback, but exact match still works" branch is verified — otherwise this branch of resolveActionPinFromHardcodedPins is not directly asserted from this test.

Before/after: subtest conversion for default MCP images

Before:

for _, image := range images {
    pin, ok := GetContainerPin(image)
    require.True(t, ok, "Expected embedded container pin for %s", image)
    assert.Equal(t, image, pin.Image, "Expected image name to match key")
    ...
}

After:

for _, image := range images {
    t.Run(image, func(t *testing.T) {
        pin, ok := GetContainerPin(image)
        require.True(t, ok, "Expected embedded container pin for %s", image)
        assert.Equal(t, image, pin.Image, "Expected image name to match key")
        ...
    })
}

2. Testify assertion upgrades

• TestFormatPinnedActionReference_PanicsWhenSHAIsEmpty and TestLoadActionPinsData_PanicsWhenEntrySHAIsEmpty use assert.Panics but never assert on the panic value/message. Use assert.PanicsWithValue (if the panic value is a known string/error) so a future refactor that changes the panic type without preserving the message doesn't silently pass.
• TestInitWarnings_InitializesAndPreservesMap subtest "preserves existing warnings map" loops with a plain for warning := range existing { assert.True(...) } — replace with assert.Subset or a direct assert.Equal(t, existing-derived-map, ctx.Warnings) to get a single clear diff on failure instead of one assertion per key silently repeating the same message.

3. Table-driven refactors

• TestResolveExactHardcodedPin_BySHA, _ByVersion, _NoMatch, _VersionTakesPrecedenceOverSHA (lines 491–528) are four separate top-level test functions covering the same function (resolveExactHardcodedPin) with near-identical structure (build one-pin slice → call → assert ok/Contains). Consolidate into one table-driven TestResolveExactHardcodedPin with a name/pins/repo/version/isSHA/wantOK/wantContains struct, consistent with the style used elsewhere in the file (e.g. TestFindCompatiblePin_SemverFallback). Reduces ~40 lines of duplication and makes it easy to add new precedence-edge-case rows later.

4. Organization/readability

• Helper type countingResolver (lines 418–425) is defined mid-file, between two unrelated test functions. Move it near the top of the file (after imports) or into a _test_helpers.go-style section so readers scanning top-to-bottom aren't interrupted mid-flow.
• Consider grouping the four resolveExactHardcodedPin tests and the two resolveNonStrictHardcodedPin tests under a shared comment banner (e.g. // --- resolveExactHardcodedPin ---) to mirror the clear separation already present around ApplyActionPinMapping/ApplyContainerPinMapping.

Acceptance Checklist

• Convert TestGetContainerPin_DefaultMCPImagesArePinned to use t.Run per image
• Add cache-correctness test for getActionPins()
• Add strict-mode exact-match case to TestResolveActionPinFromHardcodedPins_StrictModeNoFallback
• Use assert.PanicsWithValue (or equivalent) in the two panic tests
• Consolidate resolveExactHardcodedPin tests into one table-driven test
• Move countingResolver helper near imports
• make test-unit passes with no regressions

Generated by 🧪 Daily Testify Uber Super Expert · sonnet50 · 34 AIC · ⌖ 8.5 AIC · ⊞ 6.9K · ◷

• expires on Jul 30, 2026, 10:28 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I have 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.21 AIC · ⌖ 4.62 AIC · ⊞ 6.8K · ◷