[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: 28 top-level TestSpec_* functions (spec_test.go) + 23 internal functions (actionpins_internal_test.go)
Lines: spec_test.go: 778 LOC, actionpins_internal_test.go: 533 LOC, source: 514 LOC

---

Strengths

• Comprehensive table-driven tests for ExtractRepo, ExtractVersion, FormatPinnedActionReference, FormatCacheKey.
• Good coverage of ResolveActionPin paths: strict mode, enforce mode, AllowActionRefs, SkipHardcodedFallback, mappings.
• Two SPEC_MISMATCH comments document known spec/implementation divergences — valuable for future alignment.
• Internal tests exercise non-exported helpers (resolveExactHardcodedPin, resolveNonStrictHardcodedPin, buildByRepoIndex, etc.).
• Context propagation test (TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext) verifies cancellation forwarding.

---

Prioritized Improvements

1. Missing/high-value tests

1a. Dynamic resolver returning ("", nil) — no test

resolveActionPinDynamically at line 368 of actionpins.go guards on sha != "":

if err == nil && sha != "" {
    // ... success path
}
// Falls through silently → hardcoded-pin lookup next

No test exercises a resolver that returns ("", nil) — a subtle no-op condition. The fallthrough to hardcoded pins happens silently.

Before (missing): No test.

After:

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

    ctx := &actionpins.PinContext{
        Resolver: &testSHAResolver{sha: "", err: nil}, // returns empty SHA, no error
        Warnings: make(map[string]bool),
    }
    result, err := actionpins.ResolveActionPin(known, latestPin.Version, ctx)
    require.NoError(t, err, "empty-SHA resolver should fall through to hardcoded pins")
    // Should resolve via embedded pins despite resolver returning empty
    assert.Contains(t, result, latestPin.SHA, "should fall back to embedded pin when resolver returns empty SHA")
}

1b. AllowActionRefs=true combined with dynamic resolver failure — no targeted test

The table in TestSpec_PublicAPI_ResolveActionPin_EnforcePinned has a case AllowActionRefs: true but only uses the default nil resolver (pin-not-found path). There is no test combining AllowActionRefs=true + Resolver: &testSHAResolver{err: ...} + EnforcePinned: true to confirm that dynamic failure is correctly downgraded to a warning and classified as ResolutionErrorTypeDynamicResolutionFailed.

After (add to existing table):

{
    name:             "AllowActionRefs downgrades dynamic failure to warning",
    resolver:         &testSHAResolver{err: errors.New("network error")},
    allowActionRefs:  true,
    wantFailureType:  actionpins.ResolutionErrorTypeDynamicResolutionFailed,
    wantFailureCount: 1,
    wantWarningKey:   true,
},

1c. PinContext.Ctx = nil with a real resolver — nil context falls back to context.Background()

resolveActionPinDynamically calls cmp.Or(ctx.Ctx, context.Background()). There is no test where ctx.Ctx is explicitly nil and a resolver is present, verifying that context.Background() is used as the fallback (not a panic or zero context).

func TestSpec_PublicAPI_ResolveActionPin_NilCtxUsesBackground(t *testing.T) {
    known := "actions/checkout"
    resolver := &testSHAResolver{sha: testResolvedSHA}

    ctx := &actionpins.PinContext{
        Ctx:      nil, // explicitly nil
        Resolver: resolver,
        Warnings: make(map[string]bool),
    }
    _, err := actionpins.ResolveActionPin(known, "v4", ctx)
    require.NoError(t, err)
    require.NotNil(t, resolver.capturedCtx, "resolver should receive a non-nil context when PinContext.Ctx is nil")
    assert.Equal(t, context.Background(), resolver.capturedCtx,
        "nil PinContext.Ctx should fall back to context.Background()")
}

Note: context.Background() == context.Background() is only pointer-equal when the stdlib returns the same singleton. In practice verify via resolver.capturedCtx != nil rather than equality if that assumption is fragile.

1d. ActionYAMLInput.Inputs field on returned pins is never asserted

ActionPin.Inputs map[string]*ActionYAMLInput is populated from the embedded JSON and exported, but no test verifies that a pin with known inputs (e.g. actions/checkout token, path, ref inputs) returns a non-nil Inputs map with the expected entries.

func TestSpec_PublicAPI_GetActionPinsByRepo_InputsPopulated(t *testing.T) {
    pins := actionpins.GetActionPinsByRepo("actions/checkout")
    require.NotEmpty(t, pins)
    latestPin := pins[0]
    // actions/checkout is expected to expose at least "token" and "path" inputs in the embedded JSON.
    assert.NotNil(t, latestPin.Inputs, "actions/checkout pin should have Inputs populated")
    assert.Contains(t, latestPin.Inputs, "token", "expected 'token' input for actions/checkout")
}

2. Testify assertion upgrades

2a. Several assert.Equal on post-condition data that would benefit from require.Error/require.NoError earlier

In TestSpec_PublicAPI_ResolveActionPin_EnforcePinned, after the require.Error / require.NoError guards, result is checked with assert.Empty. These are fine as-is. However, in the non-table-driven tests like TestSpec_PublicAPI_ResolveActionPin_NilContext (line 225), the double-require at lines 229–230 is correct.

One pattern to avoid: assert.False(t, ok) followed by dereferencing the zero value without a require. Example in TestSpec_PublicAPI_GetLatestActionPinByRepo (line 200):

// Before
_, ok := actionpins.GetLatestActionPinByRepo("does-not-exist/unknown-action-xyzzy")
assert.False(t, ok, "should return false for unknown repo")

// After — ok; no dereferencing follows, so assert is correct here.

No urgent upgrades needed in this file — the require/assert usage is already well-structured.

2b. assert.Len(t, failures, N) should use require.Len before indexing failures[0]

In TestSpec_PublicAPI_ResolveActionPin_EnforcePinned (line 310):

// Before
require.Len(t, failures, tt.wantFailureCount, ...)
if tt.wantFailureCount > 0 {
    assert.Equal(t, tt.wantFailureType, failures[0].ErrorType)
}

This is already require.Len, which is correct (stops test if wrong length before indexing). ✅ No change needed.

3. Table-driven refactors

3a. Flatten four independent single-subtest functions into one table-driven test

These four test functions each have a single t.Run with a trivial variant that could be merged:

• TestSpec_PublicAPI_ResolveActionPin_NilContext
• TestSpec_PublicAPI_ResolveActionPin_UnknownFullSHAReturnsFormattedReference
• TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback (2 subtests — already a good pair)
• TestSpec_PublicAPI_ResolveLatestActionPin_NonNilContext

A combined TestSpec_PublicAPI_ResolveActionPin_ContextVariants table with ctx, version, wantContains, and wantEmpty columns would reduce boilerplate by ~40 lines while improving coverage visibility.

3b. TestSpec_PublicAPI_GetContainerPin has two hard-coded known images inline — could be table-driven

// Before
t.Run("returns pinned container for known image", func(t *testing.T) {
    pin, ok := actionpins.GetContainerPin("alpine:latest")
    ...
})

// After — add a table covering alpine:latest, node:lts-alpine, and an unknown image together
tests := []struct{ image string; wantOK bool }{
    {"alpine:latest", true},
    {"node:lts-alpine", true},
    {"does-not-exist/unknown-image:latest", false},
}

4. Organization/readability

4a. Two SPEC_MISMATCH comments document real behavioural divergences — should be tracked as issues

• Line 184: GetActionPinsByRepo returns nil (not empty slice) for unknown repos. The spec implies a non-nil slice. This is an observable API contract difference.
• Line 215: StrictMode=true does not cause an error for unknown pins; it returns ("", nil) silently. The spec implies an error.

These should be tracked in separate issues so the spec or implementation can be aligned. The test comments are clear, but they risk being overlooked.

4b. Rename testContextPropagationKey type alias to avoid confusion with stdlib context keys

type testContextKey string clashes visually with real context keys. Consider:

type testCtxKey struct{} // unexported struct for unambiguous type identity

However this is a minor style concern and not blocking.

---

Acceptance Checklist

• Add TestSpec_PublicAPI_ResolveActionPin_DynamicReturnsEmptySHA covering the sha == "" fallthrough path.
• Extend TestSpec_PublicAPI_ResolveActionPin_EnforcePinned table with an AllowActionRefs=true + dynamic failure row.
• Add TestSpec_PublicAPI_ResolveActionPin_NilCtxUsesBackground for ctx.Ctx = nil + resolver present.
• Add TestSpec_PublicAPI_GetActionPinsByRepo_InputsPopulated asserting Inputs map content.
• Open follow-up issues for the two SPEC_MISMATCH items (nil vs empty slice; StrictMode error semantics).
• Validate with make test-unit — all existing tests must continue to pass.

Generated by 🧪 Daily Testify Uber Super Expert · 92.1 AIC · ⌖ 20.8 AIC · ⊞ 5.2K · ◷

• expires on Jul 11, 2026, 10:39 AM UTC-08:00