diff --git a/docs/adr/42834-table-driven-subtests-for-actionpins-pin-resolution.md b/docs/adr/42834-table-driven-subtests-for-actionpins-pin-resolution.md new file mode 100644 index 00000000000..f2843442f90 --- /dev/null +++ b/docs/adr/42834-table-driven-subtests-for-actionpins-pin-resolution.md @@ -0,0 +1,44 @@ +# ADR-42834: Table-Driven Subtests for actionpins Pin-Resolution Coverage + +**Date**: 2026-07-02 +**Status**: Draft +**Deciders**: pelikhan (PR author) + +--- + +### Context + +The `pkg/actionpins` package implements critical GitHub Actions pin-resolution logic: semver fallback selection, exact-match hardcoded-pin resolution, and per-call warning deduplication via `PinContext.Warnings`. Prior to this PR, several internal branches (`findCompatiblePin`, `resolveExactHardcodedPin`, `resolveNonStrictHardcodedPin`) had no test coverage, leaving regression risk invisible. Test functions that did exist used flat top-level assertions with no subtest grouping, making failure attribution coarse-grained. The package also emits user-visible warnings to stderr as a side effect of resolution; that behavior was entirely untested. + +### Decision + +We will convert actionpins internal tests to table-driven subtests (`t.Run`) for parameterized cases and introduce `testutil.CaptureStderr` for asserting stderr side-effects of resolution functions. New dedicated test functions will cover the previously untested branches: semver major-version fallback, exact-version and exact-SHA resolution, no-match paths, and warning-dedup semantics across repeated calls. + +### Alternatives Considered + +#### Alternative 1: Flat top-level test functions (status quo) + +Continue adding one `TestX` function per scenario without subtests. Simple to write and requires no `testutil` helper. Rejected because failure output names only the top-level function, making it harder to identify which of many inputs triggered the failure; it also encourages copy-paste duplication across similar cases. + +#### Alternative 2: Interface-based stderr mock / io.Writer injection + +Inject an `io.Writer` into the resolution functions to capture output in tests, rather than adding a `testutil.CaptureStderr` helper that redirects `os.Stderr`. Considered because it avoids OS-level file descriptor manipulation in tests. Rejected because it would require changing the signatures of internal (unexported) functions or adding an exported `Writer` field to `PinContext`, widening the public surface of the package solely for test concerns. The `testutil.CaptureStderr` approach keeps production code unchanged. + +### Consequences + +#### Positive +- Subtests provide precise failure attribution (the failing subtest name identifies the exact input, e.g., `TestFindCompatiblePin_SemverFallback/exact-major`) with no extra tooling. +- Previously untested resolution branches (semver fallback, exact-match, no-match, warning dedup) are now covered, reducing regression risk for critical pin-resolution logic. +- Table-driven structure consolidates related cases (e.g., two separate `TestGetContainerPin_MCPGatewayV*` functions merged into one) and makes adding future cases low-friction. + +#### Negative +- Test file grows significantly (+170 lines), increasing the maintenance surface. +- `testutil.CaptureStderr` introduces a shared test-helper dependency; future changes to that utility can affect these tests indirectly. + +#### Neutral +- No production code (`actionpins.go`) is modified; the decision affects only the test file and the `testutil` package import. +- Adoption of `assert.Zero` over `assert.Equal(t, 0, ...)` is a stylistic cleanup bundled into this PR; no behavioral change. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go index fbae401e041..75fb4469763 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -32,15 +33,33 @@ func TestBuildByRepoIndex_GroupsByRepoAndSortsDescending(t *testing.T) { } func TestCountPinKeyMismatches_ReturnsOnlyVersionMismatches(t *testing.T) { - entries := map[string]ActionPin{ - "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"}, - "actions/setup-go@v5": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"}, - "invalid-key": {Repo: "actions/cache", Version: "v4", SHA: "sha-3"}, - } + t.Run("returns zero for empty entries", func(t *testing.T) { + assert.Zero(t, countPinKeyMismatches(map[string]ActionPin{}), "Expected empty input to produce zero mismatches") + }) - count := countPinKeyMismatches(entries) + t.Run("returns zero when all key versions match", func(t *testing.T) { + entries := map[string]ActionPin{ + "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"}, + "actions/setup-go@v4": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"}, + } + assert.Zero(t, countPinKeyMismatches(entries), "Expected zero mismatches when key versions match pin versions") + }) - assert.Equal(t, 1, count, "Expected only one key/version mismatch to be counted") + t.Run("ignores invalid keys without version suffix", func(t *testing.T) { + entries := map[string]ActionPin{ + "invalid-key": {Repo: "actions/cache", Version: "v4", SHA: "sha-3"}, + } + assert.Zero(t, countPinKeyMismatches(entries), "Expected invalid keys without @version to be ignored") + }) + + t.Run("counts only true key-version mismatches", func(t *testing.T) { + entries := map[string]ActionPin{ + "actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"}, + "actions/setup-go@v5": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"}, + "invalid-key": {Repo: "actions/cache", Version: "v4", SHA: "sha-3"}, + } + assert.Equal(t, 1, countPinKeyMismatches(entries), "Expected only one key/version mismatch to be counted") + }) } func TestInitWarnings_InitializesAndPreservesMap(t *testing.T) { @@ -113,6 +132,104 @@ func TestFormatPinnedActionWithResolution_ConsistentVersionComment(t *testing.T) } } +func TestFindCompatiblePin_SemverFallback(t *testing.T) { + pins := []ActionPin{ + {Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"}, + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"}, + {Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"}, + } + + tests := []struct { + name string + version string + wantFound bool + wantVersion string + availablePins []ActionPin + }{ + { + name: "exact-major", + version: "v5", + wantFound: true, + wantVersion: "v5.2.0", + availablePins: pins, + }, + { + // major-compatible-match: requesting v5.0.0 from a list that contains only v5.0.0 and + // v4.9.9 returns v5.0.0 — the function uses major-compatible matching, not exact-version + // matching. The only v5.x present happens to be v5.0.0, so the result looks exact. + name: "major-compatible-match", + version: "v5.0.0", + wantFound: true, + wantVersion: "v5.0.0", + availablePins: []ActionPin{ + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"}, + {Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"}, + }, + }, + { + // first-compatible-not-exact: requesting v5.0.0 from [v5.2.0, v5.0.0] returns v5.2.0 + // because findCompatiblePin returns the first major-compatible match, not the exact one. + // This is a consequence of the pre-sorted (descending) slice contract; see + // returns-first-compatible-not-highest for the complementary case with an unsorted list. + name: "first-compatible-not-exact", + version: "v5.0.0", + wantFound: true, + wantVersion: "v5.2.0", + availablePins: pins, + }, + { + // returns-first-compatible-not-highest: list order determines the result. + // With an unsorted list [v5.0.0, v5.2.0], requesting v5 returns v5.0.0 (the first + // major-compatible entry), not v5.2.0. Callers (e.g. resolveNonStrictHardcodedPin) must + // supply a pre-sorted (descending) slice via GetActionPinsByRepo to get the highest pin. + name: "returns-first-compatible-not-highest", + version: "v5", + wantFound: true, + wantVersion: "v5.0.0", + availablePins: []ActionPin{ + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-low"}, + {Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-high"}, + }, + }, + { + // minor-version-constraint: IsCompatible performs major-only comparison, so + // requesting v5.1 from [v5.2.0, v5.0.0] returns v5.2.0 (same major, first match). + name: "minor-version-constraint", + version: "v5.1", + wantFound: true, + wantVersion: "v5.2.0", + availablePins: []ActionPin{ + {Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-a"}, + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-b"}, + }, + }, + { + name: "no-match", + version: "v6", + wantFound: false, + availablePins: pins, + }, + { + name: "empty-version", + version: "", + wantFound: false, + availablePins: pins, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pin, found := findCompatiblePin(tt.availablePins, tt.version) + assert.Equal(t, tt.wantFound, found) + if tt.wantFound { + assert.Equal(t, tt.wantVersion, pin.Version) + } else { + assert.Equal(t, ActionPin{}, pin, "expected zero-value ActionPin when not found") + } + }) + } +} + 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") @@ -142,24 +259,33 @@ func TestGetContainerPin_ReturnsPinnedImage(t *testing.T) { assert.Contains(t, pin.PinnedImage, "@sha256:", "Expected pinned image to include digest") } -func TestGetContainerPin_MCPGatewayV036IsPinned(t *testing.T) { - const image = "ghcr.io/github/gh-aw-mcpg:v0.3.6" - - pin, ok := GetContainerPin(image) - require.True(t, ok, "Expected embedded container pin for %s", image) - assert.Equal(t, image, pin.Image, "Expected image name to match key") - assert.Equal(t, "sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c", pin.Digest, "Expected v0.3.6 digest to match") - assert.Equal(t, image+"@sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c", pin.PinnedImage, "Expected pinned image to include v0.3.6 digest") -} - -func TestGetContainerPin_MCPGatewayV039IsPinned(t *testing.T) { - const image = "ghcr.io/github/gh-aw-mcpg:v0.3.9" +func TestGetContainerPin_MCPGatewayVersionsArePinned(t *testing.T) { + tests := []struct { + name string + image string + digest string + }{ + { + name: "v0.3.6", + image: "ghcr.io/github/gh-aw-mcpg:v0.3.6", + digest: "sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c", + }, + { + name: "v0.3.9", + image: "ghcr.io/github/gh-aw-mcpg:v0.3.9", + digest: "sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388", + }, + } - pin, ok := GetContainerPin(image) - require.True(t, ok, "Expected embedded container pin for %s", image) - assert.Equal(t, image, pin.Image, "Expected image name to match key") - assert.Equal(t, "sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388", pin.Digest, "Expected v0.3.9 digest to match") - assert.Equal(t, image+"@sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388", pin.PinnedImage, "Expected pinned image to include v0.3.9 digest") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pin, ok := GetContainerPin(tt.image) + require.True(t, ok, "Expected embedded container pin for %s", tt.image) + assert.Equal(t, tt.image, pin.Image, "Expected image name to match key") + assert.Equal(t, tt.digest, pin.Digest, "Expected digest to match for %s", tt.image) + assert.Equal(t, tt.image+"@"+tt.digest, pin.PinnedImage, "Expected pinned image to include digest for %s", tt.image) + }) + } } func TestGetContainerPin_DefaultMCPImagesArePinned(t *testing.T) { @@ -199,7 +325,7 @@ func TestResolveActionPinDynamically_SkipsForSHAInput(t *testing.T) { assert.False(t, ok, "Expected no dynamic resolution for SHA input") assert.Empty(t, result, "Expected empty result when dynamic resolution is skipped") - assert.Equal(t, 0, resolver.called, "Expected resolver not to be called for SHA input") + assert.Zero(t, resolver.called, "Expected resolver not to be called for SHA input") } func TestResolveActionPinFromHardcodedPins_StrictModeNoFallback(t *testing.T) { @@ -220,6 +346,94 @@ func TestResolveExactHardcodedPin_BySHA(t *testing.T) { assert.Contains(t, result, "sha-v5", "Expected result to include matched SHA") } +func TestResolveExactHardcodedPin_ByVersion(t *testing.T) { + pins := []ActionPin{{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"}} + + result, ok := resolveExactHardcodedPin("actions/checkout", "v5.0.0", false, pins) + + require.True(t, ok, "Expected exact version match to resolve") + assert.Contains(t, result, "sha-v5", "Expected result to include matched SHA") + assert.Contains(t, result, "v5.0.0", "Expected result to include matched version") +} + +func TestResolveExactHardcodedPin_NoMatch(t *testing.T) { + pins := []ActionPin{{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"}} + + result, ok := resolveExactHardcodedPin("actions/checkout", "v4.0.0", false, pins) + + assert.False(t, ok, "Expected no resolution when version does not match and input is not SHA") + assert.Empty(t, result, "Expected empty result when no exact match is found") +} + +func TestResolveExactHardcodedPin_VersionTakesPrecedenceOverSHA(t *testing.T) { + // When isAlreadySHA=false, only the version-match path runs; the SHA-match loop is + // skipped entirely. This test uses a pin whose Version and SHA fields are identical + // to make the path selection explicit: the version loop matches and returns before + // the SHA loop would ever execute. + pins := []ActionPin{{Repo: "actions/checkout", Version: "sha-v5", SHA: "sha-v5"}} + result, ok := resolveExactHardcodedPin("actions/checkout", "sha-v5", false, pins) + require.True(t, ok, "Expected version-match path to resolve when isAlreadySHA=false") + assert.Contains(t, result, "sha-v5", "Expected result to include the matched pin's SHA/version") +} + +func TestResolveNonStrictHardcodedPin_SelectsHighestCompatible(t *testing.T) { + pins := []ActionPin{ + {Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"}, + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"}, + {Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"}, + } + + t.Run("uses semver-compatible pin when available", func(t *testing.T) { + ctx := &PinContext{Warnings: make(map[string]bool)} + var result string + + stderrOutput := testutil.CaptureStderr(t, func() { + result = resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx) + }) + + assert.Contains(t, result, "sha-v5-2", "Expected highest semver-compatible pin to be selected") + assert.True(t, ctx.Warnings["actions/checkout@v5"], "Expected warning cache key to be recorded") + // Assert on the stable unstyled message fragment to avoid ANSI-code fragility. + assert.Contains(t, stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0", "Expected warning to be emitted for compatible fallback") + }) + + t.Run("deduplicates warning on compatible path", func(t *testing.T) { + ctx := &PinContext{Warnings: make(map[string]bool)} + + stderrOutput := testutil.CaptureStderr(t, func() { + resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx) + resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx) + }) + + assert.Equal(t, 1, strings.Count(stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0"), + "Expected warning emitted exactly once for repeated compatible resolution") + assert.Len(t, ctx.Warnings, 1) + }) +} + +func TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible(t *testing.T) { + pins := []ActionPin{ + {Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"}, + {Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"}, + {Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"}, + } + + ctx := &PinContext{Warnings: make(map[string]bool)} + var first, second string + + stderrOutput := testutil.CaptureStderr(t, func() { + first = resolveNonStrictHardcodedPin("actions/checkout", "v9", pins, ctx) + second = resolveNonStrictHardcodedPin("actions/checkout", "v9", pins, ctx) + }) + + assert.Contains(t, first, "sha-v5-2", "Expected fallback to highest available pin when no compatible version exists") + assert.Contains(t, second, "sha-v5-2", "Expected consistent fallback result on repeated calls") + assert.True(t, ctx.Warnings["actions/checkout@v9"], "Expected warning cache key to be recorded") + assert.Len(t, ctx.Warnings, 1, "Expected warning deduplication for repeated resolution attempts") + // Assert on the stable unstyled message fragment to avoid ANSI-code fragility. + assert.Equal(t, 1, strings.Count(stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0"), "Expected warning to be emitted exactly once for repeated fallback resolution") +} + func TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback(t *testing.T) { t.Run("returns false immediately when SkipHardcodedFallback is set", func(t *testing.T) { ctx := &PinContext{SkipHardcodedFallback: true, Warnings: make(map[string]bool)}