[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: 29 top-level Test* functions (820 LOC)
Internal tests: actionpins_internal_test.go — 24 functions (562 LOC)

Strengths

• Excellent coverage of happy paths and error modes for ResolveActionPin and ResolveLatestActionPin.
• Good use of require.* for prerequisites/fatal guards and assert.* for non-fatal checks.
• Table-driven tests where appropriate.
• Concurrency test covers the sync.Once guarantee.
• Context propagation test is precise and well-structured.

---

Prioritized Improvements

1. Missing / High-Value Tests

Details

a) EnforcePinned=true + resolver succeeds

The TestSpec_PublicAPI_ResolveActionPin_EnforcePinned table covers failure scenarios but never tests the path where EnforcePinned=true and the dynamic resolver returns a valid SHA — the most important production path for enforce mode.

// After: add a success case to the existing table
{
    name:     "dynamic resolver succeeds with EnforcePinned=true returns pinned reference",
    resolver: &testSHAResolver{sha: testResolvedSHA},
    wantErr:  false,
},

b) Dynamic resolver returns empty SHA ("", nil)

The dynamic path silently falls through when the resolver returns an empty SHA with no error. This edge case is not covered:

func TestSpec_DynamicResolution_EmptySHAFallsThrough(t *testing.T) {
    ctx := &actionpins.PinContext{
        Resolver: &testSHAResolver{sha: "", err: nil},
        Warnings: make(map[string]bool),
    }
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    require.NoError(t, err)
    assert.NotEmpty(t, result, "empty SHA from resolver should fall back to hardcoded pins")
}

c) GetContainerPin — missing image@sha256: format assertion

TestSpec_PublicAPI_GetContainerPin verifies that pin.PinnedImage is non-empty but does not check the image@sha256:digest format:

// Add after the existing NotEmpty check:
assert.Truef(t, strings.Contains(pin.PinnedImage, "`@sha256`:"),
    "PinnedImage should be in image@sha256:digest format, got %q", pin.PinnedImage)

d) ctx.Ctx == nil falls back to context.Background()

TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext verifies an explicit context but no test verifies that a nil ctx.Ctx still calls the resolver without panicking.

2. Testify Assertion Upgrades

Details

Inconsistent assert.Truef vs assert.True usage (lines 473-478):

// Before:
assert.Truef(t, strings.HasPrefix(cacheKey, repo+"@"), "cache key should be repo@version, got %q", cacheKey)
assert.Truef(t, strings.HasPrefix(reference, repo+"@"), "reference should start with repo@sha, got %q", reference)
assert.Contains(t, cacheKey, version, "cache key should contain version")

// After (consistent — use f-variants only when the message has format verbs):
assert.Truef(t, strings.HasPrefix(cacheKey, repo+"@"), "cache key should be repo@version, got %q", cacheKey)
assert.Truef(t, strings.HasPrefix(reference, repo+"@"), "reference should start with repo@sha, got %q", reference)
assert.Containsf(t, cacheKey, version, "cache key should contain version")
assert.Containsf(t, reference, sha, "reference should contain sha")
assert.Containsf(t, reference, version, "reference should contain version comment")

Prefer assert.ErrorIs over assert.Contains(t, err.Error(), ...):

In TestSpec_PublicAPI_ResolveActionPin_EnforcePinned, errors are matched by substring. If the error type becomes a sentinel, prefer assert.ErrorIs; at minimum add a code comment explaining why string matching is used.

3. Table-Driven Refactors

Details

TestSpec_PublicAPI_RecordResolutionFailure and TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed share identical fixture structure (failures slice, PinContext, resolve call, length+type assertions). Merge into one table:

tests := []struct {
    name          string
    resolver      actionpins.SHAResolver
    wantErrorType actionpins.ResolutionErrorType
}{
    {
        name:          "no resolver classified as pin_not_found",
        resolver:      nil,
        wantErrorType: actionpins.ResolutionErrorTypePinNotFound,
    },
    {
        name:          "failing resolver classified as dynamic_resolution_failed",
        resolver:      &testSHAResolver{err: errors.New("network error")},
        wantErrorType: actionpins.ResolutionErrorTypeDynamicResolutionFailed,
    },
}

TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback — the three sub-tests share the same PinContext + ResolveActionPin + assertion pattern and are candidates for a struct table.

4. Organisation / Readability

Details

• testSHAResolver lacks a doc comment explaining its captured fields (capturedCtx, capturedRepo, capturedRef). Add one to clarify their role in context-propagation assertions.
• testContextPropagationKey and testResolvedSHA constants are only referenced in two tests. Add a grouping comment or move them adjacent to their consumers.
• TestSpec_Types_ActionPinsData is a struct-construction smoke test, not a behaviour test. Rename to TestSpec_Types_ActionPinsData_ConstructsCorrectly and add a one-line comment stating intent.

---

Acceptance Checklist

• Add EnforcePinned=true + resolver-succeeds case to TestSpec_PublicAPI_ResolveActionPin_EnforcePinned
• Add TestSpec_DynamicResolution_EmptySHAFallsThrough for empty-SHA resolver path
• Add @sha256: format assertion to TestSpec_PublicAPI_GetContainerPin
• Add nil-ctx.Ctx resolver test (default context fallback without panic)
• Normalise assert.Truef/assert.Containsf in TestSpec_DesignDecision_FormatConsistency
• Merge TestSpec_PublicAPI_RecordResolutionFailure* pair into a table-driven test
• Add doc comment to testSHAResolver
• Validate with make test-unit

Generated by 🧪 Daily Testify Uber Super Expert · 47.6 AIC · ⌖ 11.6 AIC · ⊞ 5.2K · ◷

• expires on Jul 20, 2026, 10:21 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 · 0.863 AIC · ⌖ 4.76 AIC · ⊞ 6.3K · ◷