[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
LOC: 792 (test) / 514 (source)

---

Strengths

• Thorough public API coverage with clear TestSpec_* naming convention.
• Correct require for preconditions, assert for value assertions.
• Context-propagation, thread-safety, and warning-deduplication all have dedicated tests.
• Consistent table-driven pattern for pure utility functions.

---

Prioritised Improvements

1. Missing / high-value tests

Details and before/after examples

1a. No test verifies RecordResolutionFailure is NOT called on success

A regression that always fires the callback would go undetected.

func TestSpec_PublicAPI_RecordResolutionFailure_NotCalledOnSuccess(t *testing.T) {
    called := false
    ctx := &actionpins.PinContext{
        Warnings: make(map[string]bool),
        RecordResolutionFailure: func(actionpins.ResolutionFailure) { called = true },
    }
    latestPin, ok := actionpins.GetLatestActionPinByRepo("actions/checkout")
    require.True(t, ok)
    _, err := actionpins.ResolveActionPin("actions/checkout", latestPin.Version, ctx)
    require.NoError(t, err)
    assert.False(t, called, "RecordResolutionFailure must not fire on successful resolution")
}

1b. Warnings = nil — verify auto-initialisation

Callers may omit Warnings. ResolveActionPin is documented to initialise it, but no test passes ctx.Warnings = nil and then inspects the map afterwards.

func TestSpec_PublicAPI_ResolveActionPin_NilWarningsAutoInitialised(t *testing.T) {
    ctx := &actionpins.PinContext{} // Warnings intentionally nil
    _, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx)
    require.NoError(t, err)
    assert.NotNil(t, ctx.Warnings, "Warnings map should be auto-initialised")
    assert.True(t, ctx.Warnings[actionpins.FormatCacheKey("does-not-exist/x", "v1")])
}

1c. Empty version string

No test exercises version = "". This is a plausible misconfiguration from malformed workflow YAML.

func TestSpec_PublicAPI_ResolveActionPin_EmptyVersion(t *testing.T) {
    ctx := &actionpins.PinContext{Warnings: make(map[string]bool)}
    result, err := actionpins.ResolveActionPin("actions/checkout", "", ctx)
    require.NoError(t, err, "empty version should not panic or error")
    _ = result // document the result contract regardless of value
}

1d. Resolver receives mapped repo/version (not original)

testSHAResolver captures capturedRepo and capturedRef, but when a Mappings redirect is in effect and a Resolver is present, the resolver should receive the mapped values — this is not currently verified.

// Add sub-test inside TestSpec_PublicAPI_ResolveActionPin_AppliesMapping:
t.Run("resolver receives mapped repo and version", func(t *testing.T) {
    resolver := &testSHAResolver{sha: testResolvedSHA}
    ctx := &actionpins.PinContext{
        Resolver: resolver,
        Warnings: make(map[string]bool),
        Mappings: map[string]string{"source/action@v1": "actions/checkout@v4"},
    }
    _, err := actionpins.ResolveActionPin("source/action", "v1", ctx)
    require.NoError(t, err)
    assert.Equal(t, "actions/checkout", resolver.capturedRepo)
    assert.Equal(t, "v4", resolver.capturedRef)
})

2. Testify assertion upgrades

Details

Promote assert.NotEmpty → require.NotEmpty at line 646

In TestSpec_PublicAPI_ResolveActionPin_DynamicHappyPath, result is asserted non-empty with assert and then immediately used in a Contains check. If the NotEmpty assertion fails, the Contains assertion runs against an empty string and produces a confusing failure message.

// Before (line 646)
assert.NotEmpty(t, result, "should return a non-empty pinned reference when resolver succeeds")
assert.Contains(t, result, latestPin.SHA, ...)

// After
require.NotEmpty(t, result, "should return a non-empty pinned reference when resolver succeeds")
assert.Contains(t, result, latestPin.SHA, ...)

Replace assert.Lenf with assert.Equalf in thread-safety test (line 626)

assert.Len checks length against an int but its failure message is less readable than assert.Equal's diff output.

// Before
assert.Lenf(t, results[i], len(results[0]), "concurrent ... goroutine %d vs 0", i)

// After
assert.Equalf(t, len(results[0]), len(results[i]), "concurrent GetActionPinsByRepo must return consistent count (goroutine %d)", i)

3. Table-driven refactor

Details

TestSpec_DynamicResolution_VersionCommentConsistency has three sub-tests with near-identical scaffolding (create resolver, create ctx, call ResolveActionPin, assert Contains). Extracting into a table removes the duplication and makes adding new cases trivial.

tests := []struct {
    name         string
    version      string
    resolverSHA  string
    wantContains []string
}{
    {
        name: "shows resolved and source version when SHA matches embedded pin",
        version: "v4", resolverSHA: latestPin.SHA,
        wantContains: []string{latestPin.SHA, latestPin.Version, "v4"},
    },
    {
        name: "shows only source version when SHA not in embedded pins",
        version: "v4", resolverSHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
        wantContains: []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "v4"},
    },
}

4. Organisation / readability

Details

• testSHAResolver has no constructor. A newTestResolver(sha string) / newErrorResolver(err error) helper would reduce ad-hoc field-setting at each call site.
• testResolvedSHA is declared at package scope but used in only one test (TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext). Either inline it or add a second usage to justify the package-level constant.

---

Acceptance Checklist

• Add TestSpec_PublicAPI_RecordResolutionFailure_NotCalledOnSuccess
• Add TestSpec_PublicAPI_ResolveActionPin_NilWarningsAutoInitialised
• Add TestSpec_PublicAPI_ResolveActionPin_EmptyVersion
• Add resolver-receives-mapped-repo/version sub-test inside TestSpec_PublicAPI_ResolveActionPin_AppliesMapping
• Promote assert.NotEmpty → require.NotEmpty at line 646
• Replace assert.Lenf → assert.Equalf in thread-safety test (line 626)
• Optionally refactor TestSpec_DynamicResolution_VersionCommentConsistency to table-driven
• All changes pass make test-unit

Generated by 🧪 Daily Testify Uber Super Expert · 40.4 AIC · ⌖ 12.7 AIC · ⊞ 5.1K · ◷

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

[github-actions]

This issue was automatically closed because it expired on 2026-07-16T18:24:50.814Z.

Closed by Workflow