[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 functions: 27 | LOC: 845

The internal test file is well-structured with many table-driven tests and good use of require for prerequisites. The issues below are concrete and addressable.

---

Strengths

• Table-driven tests throughout (TestFindCompatiblePin_SemverFallback, TestApplyActionPinMapping, TestApplyContainerPinMapping).
• Subtest naming follows "verb - condition" patterns, aiding readability.
• Good use of require.True/NotEmpty as guards before accessing values (e.g., lines 316, 365, 583).

---

Prioritized Improvements

1. Missing / high-value tests

ApplyContainerPinMapping has no black-box spec test — it is exported but only tested internally. The companion spec_test.go covers all other exported symbols. Add one spec-level test mirroring the internal table structure to ensure the public contract is documented and will catch regressions if the function is refactored into a new package.

formatPinnedActionWithResolution — missing table row for sourceVersion == "" with a non-empty resolvedVersion. The current table has:

• source-only (resolved empty) ✓
• source == resolved ✓
• source differs from resolved ✓

Missing: sourceVersion="", resolvedVersion="v5.2.0" — it is unclear what the expected output is. This is a specification gap worth pinning.

2. Testify assertion upgrades — assert used where require is appropriate

Several places use assert for conditions where a failure makes subsequent assertions in the same sub-test meaningless:

Lines to change from assert → require

Line 303 — TestFindCompatiblePin_SemverFallback loop body:

// Before
assert.Equal(t, tt.wantFound, found)
if tt.wantFound {
    assert.Equal(t, tt.wantVersion, pin.Version)

// After
require.Equal(t, tt.wantFound, found)
if tt.wantFound {
    assert.Equal(t, tt.wantVersion, pin.Version)

wantFound is the gate condition for the pin.Version check; if it fails, pin is the zero value and the message is confusing.

Lines 600–601 — TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback subtest "allows hardcoded pins when SkipHardcodedFallback is not set":

// Before
assert.True(t, ok, "Expected hardcoded pins to be consulted when SkipHardcodedFallback is false")
assert.NotEmpty(t, result, ...)

// After
require.True(t, ok, "Expected hardcoded pins to be consulted when SkipHardcodedFallback is false")
assert.NotEmpty(t, result, ...)

ok gates the value of result; if ok is false, result is empty by contract, so the second assertion adds noise.

Line 537 — TestResolveNonStrictHardcodedPin_SelectsHighestCompatible "deduplicates warning on compatible path":

// Before
assert.Len(t, ctx.Warnings, 1)

// After
require.Len(t, ctx.Warnings, 1, "expected exactly one warning key after two calls")

No message is provided, making failures hard to diagnose. Upgrade to require with message since the test has a single purpose.

3. Table-driven refactor

TestFindVersionBySHA_ReturnsVersionForKnownSHA uses three nested t.Run subtests with near-identical setup but is not table-driven. The "returns empty string for unknown SHA" and "returns empty string for unknown repo" cases can be collapsed into a small []struct{ name, repo, sha, want string } table, removing ~15 LOC of duplication.

Example refactor

// Before — three separate subtests
func TestFindVersionBySHA_ReturnsVersionForKnownSHA(t *testing.T) {
    t.Run("returns version for a known SHA in embedded data", func(t *testing.T) {
        pins := GetActionPinsByRepo("actions/checkout")
        require.NotEmpty(t, pins, "prerequisite: embedded pins must exist")
        knownPin := pins[0]
        version := findVersionBySHA("actions/checkout", knownPin.SHA)
        assert.Equal(t, knownPin.Version, version, "should return the version for a known SHA")
    })
    t.Run("returns empty string for unknown SHA", func(t *testing.T) {
        version := findVersionBySHA("actions/checkout", "0000...")
        assert.Empty(t, version, ...)
    })
    t.Run("returns empty string for unknown repo", func(t *testing.T) {
        version := findVersionBySHA("does-not-exist/unknown", "abc123")
        assert.Empty(t, version, ...)
    })
}

// After — table-driven for the negative cases; keep the embedded-data lookup as a setup step
func TestFindVersionBySHA(t *testing.T) {
    pins := GetActionPinsByRepo("actions/checkout")
    require.NotEmpty(t, pins)
    knownPin := pins[0]

    tests := []struct {
        name    string
        repo    string
        sha     string
        want    string
    }{
        {"known SHA returns version", "actions/checkout", knownPin.SHA, knownPin.Version},
        {"unknown SHA returns empty", "actions/checkout", "0000000000000000000000000000000000000000", ""},
        {"unknown repo returns empty", "does-not-exist/unknown", "abc123", ""},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            assert.Equal(t, tt.want, findVersionBySHA(tt.repo, tt.sha))
        })
    }
}

4. Organization / readability

• TestResolveNonStrictHardcodedPin_SelectsHighestCompatible line 537: assert.Len(t, ctx.Warnings, 1) has no failure message. All other similar assertions in the file include one. Add a descriptive message.
• TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible: The function is tested with a flat (non-subtested) body even though it exercises two distinct scenarios (first call result and second call deduplication). Consider wrapping in subtests for clarity.

---

Acceptance Checklist

• assert.Equal(t, tt.wantFound, found) in TestFindCompatiblePin_SemverFallback upgraded to require
• assert.True(t, ok, ...) / assert.NotEmpty(t, result, ...) in TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback upgraded to require
• assert.Len(t, ctx.Warnings, 1) in deduplication subtest given a descriptive failure message (and consider require)
• TestFindVersionBySHA_ReturnsVersionForKnownSHA refactored to table-driven
• New table row added to TestFormatPinnedActionWithResolution_ConsistentVersionComment for sourceVersion="" + non-empty resolvedVersion
• New spec-level test in spec_test.go for exported ApplyContainerPinMapping (black-box, at least: nil ctx, no mapping, valid mapping)
• All changes validated with make test-unit

References: §30033266054

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

• expires on Jul 25, 2026, 10:30 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 · gpt54 · 2.98 AIC · ⌖ 6.73 AIC · ⊞ 8.1K · ◷