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

[github-actions]

Overview

File: pkg/actionpins/spec_test.go
Paired source: pkg/actionpins/actionpins.go
Test functions: 29 top-level | LOC: 711 test / 514 source

Strengths

• Good require.*/assert.* separation; clean testSHAResolver stub; honest SPEC_MISMATCH annotations; solid table-driven coverage for pure functions; thread-safety test for sync.Once.

---

1. Missing / High-Value Tests

1a. SHA-as-version fallback (source lines 330–333) — untested

When an unknown 40-char SHA is passed as version, the source returns "repo@<sha> # <sha>". No test covers this path.

func TestSpec_PublicAPI_ResolveActionPin_UnknownSHAPassthrough(t *testing.T) {
    unknownSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
    result, err := actionpins.ResolveActionPin("actions/checkout", unknownSHA, nil)
    require.NoError(t, err)
    expected := actionpins.FormatPinnedActionReference("actions/checkout", unknownSHA, unknownSHA)
    assert.Equal(t, expected, result, "unknown SHA should produce repo@sha # sha")
}

1b. Invalid mapping value silently skipped (source lines 497–500) — untested

A malformed mapping value (e.g. missing @, empty repo) is logged and skipped. No test verifies resolution falls through unchanged.

func TestSpec_PublicAPI_ResolveActionPin_InvalidMappingSkipped(t *testing.T) {
    ctx := &actionpins.PinContext{
        Warnings: make(map[string]bool),
        Mappings: map[string]string{"actions/checkout@v4": "actions/checkout"}, // no `@version`
    }
    result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
    require.NoError(t, err, "invalid mapping should not cause an error")
    assert.Contains(t, result, "actions/checkout@", "invalid mapping should leave resolution unchanged")
}

1c. ResolveLatestActionPin error-path fallback (source lines 474–476) — untested

When ResolveActionPin errors (e.g. EnforcePinned=true, unknown repo), ResolveLatestActionPin falls back to getLatestActionPinReference. No test verifies this.

func TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError(t *testing.T) {
    ctx := &actionpins.PinContext{EnforcePinned: true, Warnings: make(map[string]bool)}
    result := actionpins.ResolveLatestActionPin("does-not-exist/x", ctx)
    assert.Empty(t, result, "unknown repo with enforce mode should return empty from fallback")
}

1d. PinContext.Ctx propagated to resolver — untested

The documented nil-fallback to context.Background() path is exercised by other tests, but no test verifies that a non-nil (e.g. already-cancelled) context is forwarded to the resolver.

---

2. Testify Assertion Upgrades

2a. Upgrade assert.NotEmpty → require.NotEmpty before chained content assertions

In TestSpec_PublicAPI_ResolveActionPin_DynamicHappyPath, TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatch, and the TestSpec_DynamicResolution_VersionCommentConsistency sub-tests, assert.NotEmpty(result) is followed by assert.Contains(result, ...). If result is empty, the Contains assertions produce confusing noise.

// Before:
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, ...)

---

3. Table-Driven Refactors

3a. Consolidate TestSpec_PublicAPI_ResolveActionPin_EnforcePinned sub-tests

The 3 sub-tests each build a PinContext nearly identically. Extract into a table with fields: resolver, allowActionRefs, wantErr, wantErrContains, wantFailureType, wantFailureCount.

---

4. Organization / Readability

4a. Thread-safety test: use sync.WaitGroup instead of manual channel

// Before: manual done channel
done := make(chan int, goroutines)
for i := range goroutines {
    go func(idx int) { results[idx] = ...; done <- idx }(i)
}
for range goroutines { <-done }

// After: idiomatic
var wg sync.WaitGroup
for i := range goroutines {
    wg.Add(1)
    go func(idx int) { defer wg.Done(); results[idx] = ... }(i)
}
wg.Wait()

4b. Consider removing struct-field-only tests

TestSpec_Types_ActionPin, TestSpec_Types_ActionYAMLInput, TestSpec_Types_ContainerPin, and TestSpec_Types_ResolutionFailure only construct structs and read field values. They test nothing behavioral and will require updates on any field rename. Replace with compile-time assertions or remove.

---

Acceptance Checklist

• Add TestSpec_PublicAPI_ResolveActionPin_UnknownSHAPassthrough
• Add TestSpec_PublicAPI_ResolveActionPin_InvalidMappingSkipped
• Add TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError
• Add context-propagation test with a context-capturing resolver stub
• Upgrade assert.NotEmpty → require.NotEmpty before chained content assertions (3 tests)
• Refactor TestSpec_PublicAPI_ResolveActionPin_EnforcePinned into a table-driven test
• Replace done chan with sync.WaitGroup in the thread-safety test
• Remove or replace struct-only tests for ActionPin, ActionYAMLInput, ContainerPin, ResolutionFailure

Validate with: go test ./pkg/actionpins/... -v -count=1

Generated by 🧪 Daily Testify Uber Super Expert · 75.4 AIC · ⌖ 10.6 AIC · ⊞ 5.2K · ◷

• expires on Jul 8, 2026, 10:41 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 · 1.43 AIC · ⌖ 7.97 AIC · ⊞ 4.6K · ◷