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

[github-actions]

Overview

File: pkg/actionpins/actionpins_internal_test.go
Source pair: pkg/actionpins/actionpins.go
Test count: 27 (internal) + 30 (spec_test.go) = 57 total
LOC: 615 (internal test), 900 (spec test), 535 (source)

The test suite is comprehensive and well-structured with thoughtful coverage of the public API, concurrency, edge cases, and regression scenarios. The improvements below focus on the remaining gaps.

---

Strengths

• Excellent table-driven tests for findCompatiblePin, FormatPinnedActionReference, ExtractRepo/Version.
• Good regression tests with clear prose comments (e.g., SHA→version comment fix).
• assert vs require used correctly: setup/prerequisite steps use require, subsequent checks use assert.
• Thread-safety test for concurrent GetActionPinsByRepo.
• Warning deduplication verified both in internal and spec tests.

---

Prioritized Improvements

1. Missing / High-Value Tests

1a. getLatestActionPinReference — internal helper has no direct test

getLatestActionPinReference (line 246 in actionpins.go) is only indirectly tested through ResolveLatestActionPin. A direct internal test would verify the fallback contract:

// Before (no test)

// After — add to actionpins_internal_test.go
func TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmpty(t *testing.T) {
    t.Run("returns formatted reference for known repo", func(t *testing.T) {
        latestPin, ok := GetLatestActionPinByRepo("actions/checkout")
        require.True(t, ok)
        result := getLatestActionPinReference("actions/checkout")
        assert.Equal(t, FormatPinnedActionReference("actions/checkout", latestPin.SHA, latestPin.Version), result)
    })
    t.Run("returns empty string for unknown repo", func(t *testing.T) {
        result := getLatestActionPinReference("does-not-exist/x")
        assert.Empty(t, result)
    })
}

1b. recordPinResolutionFailure — nil ctx and nil callback branches untested directly

The nil-guard branches in recordPinResolutionFailure are only exercised implicitly. Direct tests would guard against regressions:

// After — add to actionpins_internal_test.go
func TestRecordPinResolutionFailure_NilSafety(t *testing.T) {
    t.Run("does not panic with nil ctx", func(t *testing.T) {
        assert.NotPanics(t, func() {
            recordPinResolutionFailure(nil, "actions/checkout", "v4", ResolutionErrorTypePinNotFound)
        })
    })
    t.Run("does not panic with nil callback", func(t *testing.T) {
        ctx := &PinContext{Warnings: make(map[string]bool)}
        assert.NotPanics(t, func() {
            recordPinResolutionFailure(ctx, "actions/checkout", "v4", ResolutionErrorTypePinNotFound)
        })
    })
    t.Run("calls callback when non-nil", func(t *testing.T) {
        var recorded []ResolutionFailure
        ctx := &PinContext{
            Warnings: make(map[string]bool),
            RecordResolutionFailure: func(f ResolutionFailure) { recorded = append(recorded, f) },
        }
        recordPinResolutionFailure(ctx, "actions/checkout", "v4", ResolutionErrorTypePinNotFound)
        require.Len(t, recorded, 1)
        assert.Equal(t, "actions/checkout", recorded[0].Repo)
        assert.Equal(t, "v4", recorded[0].Ref)
        assert.Equal(t, ResolutionErrorTypePinNotFound, recorded[0].ErrorType)
    })
}

1c. logDynamicResolutionSkipped — no test for the no-resolver branch

The hasResolver=false, isAlreadySHA=false log-only branch in logDynamicResolutionSkipped is never directly exercised:

// After — add to actionpins_internal_test.go
func TestLogDynamicResolutionSkipped_NoResolverBranch(t *testing.T) {
    // logDynamicResolutionSkipped is a logging-only function; verify it does not panic.
    assert.NotPanics(t, func() {
        logDynamicResolutionSkipped(false, false) // no resolver, not a SHA
        logDynamicResolutionSkipped(false, true)  // not a resolver, is a SHA
        logDynamicResolutionSkipped(true, true)   // has resolver, is a SHA
    })
}

1d. loadActionPinsData — valid JSON path not tested for containers

TestLoadActionPinsData_PanicsWhenEntrySHAIsEmpty only verifies the panic path. A success-path test for the container Pins field would improve branch coverage:

func TestLoadActionPinsData_LoadsContainerPins(t *testing.T) {
    fixture := []byte(`{
        "entries": {
            "actions/checkout@v5": {"repo":"actions/checkout","version":"v5","sha":"abc123"}
        },
        "containers": {
            "node:lts-alpine": {"image":"node:lts-alpine","digest":"sha256:dead","pinnedImage":"node:lts-alpine@sha256:dead"}
        }
    }`)
    data := loadActionPinsData(fixture)
    require.Len(t, data.Entries, 1)
    require.Len(t, data.Containers, 1)
    assert.Equal(t, "sha256:dead", data.Containers["node:lts-alpine"].Digest)
}

2. Testify Assertion Upgrades

Use require.Len instead of assert.Len where the loop below depends on the length

In TestBuildByRepoIndex_GroupsByRepoAndSortsDescending (line 27–33), require.Len is already used ✅.

In TestGetContainerPin_MCPGatewayVersionsArePinned (line 325), after require.NotEmpty, the per-subtest require.True(t, ok) is correct. However:

// Before (line 334):
assert.Equal(t, image+"@"+pin.Digest, pin.PinnedImage, ...)

// After — no change needed; assert is correct since prior require.True gates the block

No urgent upgrades required here — the existing assert/require split is sound.

3. Table-Driven Refactors

3a. Collapse TestApplyActionPinMapping_* into a single table-driven test

There are 5 separate TestApplyActionPinMapping_* functions (lines 526–614) testing permutations of the same function. Consolidating into a table-driven test improves readability and makes it easier to add new cases:

// Before: 5 separate functions (~90 LOC)

// After: one table-driven test (~60 LOC)
func TestApplyActionPinMapping(t *testing.T) {
    tests := []struct {
        name            string
        mappings        map[string]string
        inputRepo       string
        inputVersion    string
        wantRepo        string
        wantVersion     string
        wantWarningKey  string
        wantWarningSet  bool
    }{
        {
            name:         "no mapping leaves repo and version unchanged",
            inputRepo:    "actions/checkout", inputVersion: "v4",
            wantRepo: "actions/checkout", wantVersion: "v4",
        },
        {
            name:     "valid mapping redirects repo and version",
            mappings: map[string]string{"actions/checkout@v4": "acme-corp/checkout@v4"},
            inputRepo: "actions/checkout", inputVersion: "v4",
            wantRepo: "acme-corp/checkout", wantVersion: "v4",
            wantWarningKey: "map:actions/checkout@v4", wantWarningSet: true,
        },
        // ... other cases
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            ctx := &PinContext{Warnings: make(map[string]bool), Mappings: tt.mappings}
            repo, version := applyActionPinMapping(tt.inputRepo, tt.inputVersion, ctx)
            assert.Equal(t, tt.wantRepo, repo)
            assert.Equal(t, tt.wantVersion, version)
            if tt.wantWarningKey != "" {
                assert.Equal(t, tt.wantWarningSet, ctx.Warnings[tt.wantWarningKey])
            }
        })
    }
}

4. Organization / Readability

4a. Add build tag comment to spec_test.go explaining external package choice

actionpins_internal_test.go has package actionpins (white-box) while spec_test.go has package actionpins_test (black-box). A short comment at the top of spec_test.go clarifying the intentional split would help future contributors:

// Package actionpins_test validates the public API surface of the actionpins package.
// White-box (internal) tests live in actionpins_internal_test.go.
package actionpins_test

4b. Extract repeated PinContext construction into a helper

Many tests construct &actionpins.PinContext{Warnings: make(map[string]bool)}. A small helper reduces boilerplate:

// In spec_test.go (or a testhelper file)
func newCtx(opts ...func(*actionpins.PinContext)) *actionpins.PinContext {
    ctx := &actionpins.PinContext{Warnings: make(map[string]bool)}
    for _, o := range opts {
        o(ctx)
    }
    return ctx
}
// Usage:
ctx := newCtx(func(c *actionpins.PinContext) { c.StrictMode = true })

---

Acceptance Checklist

• Add TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmpty to actionpins_internal_test.go
• Add TestRecordPinResolutionFailure_NilSafety to actionpins_internal_test.go
• Add TestLogDynamicResolutionSkipped_NoResolverBranch to actionpins_internal_test.go
• Add TestLoadActionPinsData_LoadsContainerPins to actionpins_internal_test.go
• Consolidate TestApplyActionPinMapping_* into a single table-driven test
• Add package-level comment to spec_test.go
• Optionally extract newCtx helper in spec_test.go
• Run make test-unit to confirm all tests pass

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

• expires on Jul 23, 2026, 10:29 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! 🍪

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pi.dev

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pi.dev"

See Network Configuration for more information.

🍪 Om nom nom by Issue Monster · gpt54 2.32 AIC · ⌖ 7.13 AIC · ⊞ 8.1K · ◷