[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 count: 30 top-level test functions
LOC: ~901
Testify usage: require.* (52 calls) + assert.* (77 calls)

Strengths

• Good use of table-driven tests with t.Run throughout
• require used correctly for gate conditions (e.g., require.True(t, ok, ...) before using the returned value)
• Context propagation tested with a custom testContextKey type
• Thread safety covered via TestSpec_ThreadSafety_ConcurrentGetActionPinsByRepo
• Edge cases for ExtractRepo/ExtractVersion are thorough (empty string, multiple @, leading @)

---

Prioritized Improvements

1. Missing / High-Value Tests

Details

FormatPinnedActionReference — empty SHA or empty version not tested

The table in TestSpec_PublicAPI_FormatPinnedActionReference covers happy-path cases but does not test degenerate inputs where sha or version is empty. The spec comment says the format is repo@sha # version; it is unclear what the function emits when version is "".

// Missing case — add to existing tests slice:
{
    name:     "empty version omits comment",
    repo:     "actions/checkout",
    sha:      "abc123",
    version:  "",
    expected: "actions/checkout@abc123", // or "actions/checkout@abc123 # " — assert the actual contract
},

GetContainerPin — only one known image tested

TestSpec_PublicAPI_GetContainerPin tests a single knownImage built from constants.DefaultMCPGatewayContainer. If the embedded data ever has more containers, none of the additional entries are exercised. Consider a loop over all embedded containers asserting the digest format invariant.

PinContext.Mappings — self-mapping / cycle not tested

TestSpec_PublicAPI_ResolveActionPin_AppliesMapping tests valid mapping and mapping-to-unknown but does not test a mapping that maps an action to itself (a no-op cycle). This is an easy input that could cause a subtle loop if the implementation ever changes.

t.Run("self-mapping is a no-op", func(t *testing.T) {
    ctx := &actionpins.PinContext{
        Warnings: make(map[string]bool),
        Mappings: map[string]string{
            "actions/checkout@v4": "actions/checkout@v4", // maps to itself
        },
    }
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    require.NoError(t, err)
    require.NotEmpty(t, result, "self-mapping should still resolve from embedded pins")
})

2. Testify Assertion Upgrades (assert → require where failure invalidates subsequent assertions)

Details

Line ~754 — assert.NoError inside require.NotPanics

// Current
require.NotPanics(t, func() {
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    assert.NoError(t, err)      // ← if err != nil, result is meaningless
    assert.NotEmpty(t, result)
}, "nil PinContext.Ctx should fall back to context.Background() without panicking")

// Improved
require.NotPanics(t, func() {
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    require.NoError(t, err)
    assert.NotEmpty(t, result)
}, "nil PinContext.Ctx should fall back to context.Background() without panicking")

Note: require inside a closure passed to NotPanics calls t.FailNow() on the test goroutine; this is intentional and correct here.

Line ~898 — same pattern in TestSpec_PublicAPI_ResolveActionPin_MappingTargetUnknown

// Current
require.NotPanics(t, func() {
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    assert.NoError(t, err)     // ← upgrade to require
    assert.Empty(t, result, "mapping to unknown repo should produce unresolved empty result")
})

// Improved
require.NotPanics(t, func() {
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    require.NoError(t, err)
    assert.Empty(t, result, "mapping to unknown repo should produce unresolved empty result")
})

Line ~344 — assert.Equal on failures[0].ErrorType without guarding len(failures) > 0

require.Len is called just before, so this is safe today. No change strictly needed, but adding a comment that the require.Len is the guard improves readability.

3. Table-Driven Refactors

Details

TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatch is a standalone function, not table-driven

This test (line ~392) tests a single SHA-based lookup. It could be merged into TestSpec_PublicAPI_ResolveActionPin as an additional table row, or at minimum given a t.Run wrapper so it shows up named in test output:

t.Run("known SHA resolves from embedded match with version comment", func(t *testing.T) {
    // existing body
})

TestSpec_PublicAPI_ResolveActionPin_NilContext and _NilCtxField are structurally similar

They could be combined into a small table:

tests := []struct{
    name string
    ctx  *actionpins.PinContext
}{
    {"nil context pointer", nil},
    {"non-nil context with nil Ctx field", &actionpins.PinContext{Resolver: resolver, Warnings: make(map[string]bool)}},
}

4. Organization / Readability

Details

• The testSHAResolver helper struct is defined near the top of the file. Adding a one-line doc comment (// testSHAResolver is a controllable SHAResolver for use in table-driven tests.) would clarify intent.
• testContextPropagationKey constant is used only by TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext. Co-locating it near that test or making it a local variable inside the test reduces file-level pollution.
• The TestSpec_DesignDecision_FormatConsistency test uses bare assert calls at the top level of the test function (not inside a t.Run). Given the test verifies a cross-cutting design constraint, this is acceptable, but a top-level comment explaining what "design decision" means in this context would help future readers.

---

Acceptance Checklist

• Add missing FormatPinnedActionReference edge-case table rows (empty version, empty SHA)
• Add self-mapping test to TestSpec_PublicAPI_ResolveActionPin_AppliesMapping
• Upgrade assert.NoError → require.NoError inside the two require.NotPanics closures
• Wrap TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatch body in a t.Run
• All changes pass make test-unit

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

• expires on Jul 24, 2026, 10:26 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 2.18 AIC · ⌖ 9.47 AIC · ⊞ 7.6K · ◷