diff --git a/docs/adr/47206-table-driven-tests-and-internal-coverage-for-actionpins.md b/docs/adr/47206-table-driven-tests-and-internal-coverage-for-actionpins.md new file mode 100644 index 00000000000..c6443353c6d --- /dev/null +++ b/docs/adr/47206-table-driven-tests-and-internal-coverage-for-actionpins.md @@ -0,0 +1,45 @@ +# ADR-47206: Table-Driven Tests and Internal Coverage for actionpins + +**Date**: 2026-07-22 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +The `pkg/actionpins` package is a core library responsible for resolving, mapping, and pinning GitHub Actions references and container images. Its internal test file (`actionpins_internal_test.go`) had grown a set of per-scenario test functions for `applyActionPinMapping` (six separate `TestApplyActionPinMapping_*` functions), each with duplicated `PinContext` setup boilerplate. Additionally, several internal helper functions — `getLatestActionPinReference`, `recordPinResolutionFailure`, `logDynamicResolutionSkipped`, and the container-pin branch of `loadActionPinsData` — had no direct test coverage, leaving edge-case nil-safety and boundary behaviors unverified. The PR #47206 was opened to close these gaps and improve test maintainability. + +### Decision + +We will consolidate the `applyActionPinMapping` per-scenario tests into a single table-driven `TestApplyActionPinMapping` function, and add four new internal test functions targeting previously uncovered code paths (`TestLoadActionPinsData_LoadsContainerPins`, `TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmpty`, `TestLogDynamicResolutionSkipped_NoResolverBranch`, `TestRecordPinResolutionFailure_NilSafety`). The table-driven approach is chosen because it is idiomatic Go, reduces boilerplate for each new scenario, and surfaces all cases in a single view, making it easier to audit coverage. + +### Alternatives Considered + +#### Alternative 1: Continue Adding Per-Scenario Test Functions + +The status quo approach — each mapping scenario gets its own top-level `TestApplyActionPinMapping_*` function. Why considered: matches the existing style and requires no refactoring. Why not chosen: the six existing functions already contain significant boilerplate duplication; adding further scenarios would compound maintenance cost and make it harder to see at a glance which cases are covered. The `repeat` field needed for the deduplication scenario is also awkward to express without the table struct. + +#### Alternative 2: Move Missing Coverage to Black-Box Spec Tests in spec_test.go + +Add coverage for `getLatestActionPinReference`, `recordPinResolutionFailure`, and `logDynamicResolutionSkipped` in the `actionpins_test` (black-box) package rather than the internal test package. Why considered: black-box tests are less brittle because they don't depend on unexported symbols. Why not chosen: the functions under test are unexported (`getLatestActionPinReference`, `recordPinResolutionFailure`, `logDynamicResolutionSkipped`); they cannot be called from `actionpins_test`. Internal tests are the only way to exercise them directly, and the nil-safety behavior is only observable via those unexported entry points. + +### Consequences + +#### Positive +- Adding new `applyActionPinMapping` scenarios now requires adding a single struct literal to the test table rather than a new top-level function, lowering the per-scenario authoring cost. +- Previously untested nil-safety and no-resolver branches are now explicitly verified, reducing the risk of panics in production edge cases (nil `PinContext`, nil `RecordResolutionFailure` callback). +- The container-pin branch of `loadActionPinsData` is now independently verified without relying on the embedded data fixture. +- A package-level doc comment added to `spec_test.go` clarifies the purpose of the black-box test module for future contributors. + +#### Negative +- Table-driven tests with a large number of fields (`name`, `actionRepo`, `version`, `mappings`, `repeat`, `wantRepo`, `wantVersion`, `wantMappingNotification`, `wantMapNotificationKeys`) can be harder to scan when a single case fails — the failure message must be read alongside the struct to reconstruct the intent. +- The `repeat` field is non-obvious: it exists solely for the deduplication scenario; all other cases default to `repeat = 1` via `max(tt.repeat, 1)`. This implicit default must be understood by future editors. + +#### Neutral +- No production code was changed; test-only changes do not affect binary artifacts or runtime behavior. +- The consolidation deletes 79 lines and adds 181, for a net increase of 102 lines — the expanded table carries more information than the removed boilerplate. + +--- + +*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 3f2363e3b4a..88fc851f8f2 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -110,6 +110,32 @@ func TestLoadActionPinsData_PanicsWhenEntrySHAIsEmpty(t *testing.T) { }, "Expected loadActionPinsData to panic when embedded pin data contains an empty SHA") } +func TestLoadActionPinsData_LoadsContainerPins(t *testing.T) { + fixture := []byte(`{ + "entries": { + "actions/checkout@v5": { + "repo": "actions/checkout", + "version": "v5", + "sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + }, + "containers": { + "node:lts-alpine": { + "image": "node:lts-alpine", + "digest": "sha256:deadbeef", + "pinned_image": "node:lts-alpine@sha256:deadbeef" + } + } + }`) + + data := loadActionPinsData(fixture) + + require.Contains(t, data.Containers, "node:lts-alpine", "Expected container pin key to be loaded") + assert.Equal(t, "node:lts-alpine", data.Containers["node:lts-alpine"].Image) + assert.Equal(t, "sha256:deadbeef", data.Containers["node:lts-alpine"].Digest) + assert.Equal(t, "node:lts-alpine@sha256:deadbeef", data.Containers["node:lts-alpine"].PinnedImage) +} + func TestFormatPinnedActionReference_PanicsWhenSHAIsEmpty(t *testing.T) { assert.Panics(t, func() { FormatPinnedActionReference("ruby/setup-ruby", "", "v1.319.0") @@ -305,6 +331,20 @@ func TestFindVersionBySHA_ReturnsVersionForKnownSHA(t *testing.T) { }) } +func TestGetLatestActionPinReference_ReturnsFormattedReferenceOrEmpty(t *testing.T) { + t.Run("returns formatted reference for known repo", func(t *testing.T) { + pins := GetActionPinsByRepo("actions/checkout") + require.NotEmpty(t, pins, "prerequisite: embedded pins must exist for actions/checkout") + + result := getLatestActionPinReference("actions/checkout") + assert.Equal(t, FormatPinnedActionReference("actions/checkout", pins[0].SHA, pins[0].Version), result) + }) + + t.Run("returns empty string for unknown repo", func(t *testing.T) { + assert.Empty(t, getLatestActionPinReference("does-not-exist/unknown")) + }) +} + func TestGetContainerPin_ReturnsPinnedImage(t *testing.T) { pin, ok := GetContainerPin("node:lts-alpine") require.True(t, ok, "Expected embedded container pin for node:lts-alpine") @@ -376,6 +416,45 @@ func TestResolveActionPinDynamically_SkipsForSHAInput(t *testing.T) { assert.Zero(t, resolver.called, "Expected resolver not to be called for SHA input") } +func TestLogDynamicResolutionSkipped_NoResolverBranch(t *testing.T) { + assert.NotPanics(t, func() { + logDynamicResolutionSkipped(false, false) + }, "Expected no-resolver branch to be safe") +} + +func TestRecordPinResolutionFailure_NilSafety(t *testing.T) { + t.Run("nil context is safe", func(t *testing.T) { + assert.NotPanics(t, func() { + recordPinResolutionFailure(nil, "actions/checkout", "v4", ResolutionErrorTypePinNotFound) + }) + }) + + t.Run("nil callback is safe", func(t *testing.T) { + ctx := &PinContext{} + assert.NotPanics(t, func() { + recordPinResolutionFailure(ctx, "actions/checkout", "v4", ResolutionErrorTypePinNotFound) + }) + }) + + t.Run("records failure when callback is configured", func(t *testing.T) { + var got []ResolutionFailure + ctx := &PinContext{ + RecordResolutionFailure: func(f ResolutionFailure) { + got = append(got, f) + }, + } + + recordPinResolutionFailure(ctx, "actions/checkout", "v4", ResolutionErrorTypeDynamicResolutionFailed) + + require.Len(t, got, 1, "Expected one resolution failure record") + assert.Equal(t, ResolutionFailure{ + Repo: "actions/checkout", + Ref: "v4", + ErrorType: ResolutionErrorTypeDynamicResolutionFailed, + }, got[0]) + }) +} + func TestResolveActionPinFromHardcodedPins_StrictModeNoFallback(t *testing.T) { ctx := &PinContext{StrictMode: true, Warnings: make(map[string]bool)} @@ -523,93 +602,116 @@ func TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback(t *testing.T) { }) } -func TestApplyActionPinMapping_NoMapping(t *testing.T) { - ctx := &PinContext{Warnings: make(map[string]bool)} - - repo, version := applyActionPinMapping("actions/checkout", "v4", ctx) - - assert.Equal(t, "actions/checkout", repo, "repo should be unchanged when no mapping exists") - assert.Equal(t, "v4", version, "version should be unchanged when no mapping exists") -} - -func TestApplyActionPinMapping_AppliesMapping(t *testing.T) { - ctx := &PinContext{ - Warnings: make(map[string]bool), - Mappings: map[string]string{ - "actions/checkout@v4": "acme-corp/checkout@v4", +func TestApplyActionPinMapping(t *testing.T) { + tests := []struct { + name string + actionRepo string + version string + mappings map[string]string + repeat int + wantRepo string + wantVersion string + wantMappingNotification bool + wantMapNotificationKeys int + }{ + { + name: "no mapping", + actionRepo: "actions/checkout", + version: "v4", + wantRepo: "actions/checkout", + wantVersion: "v4", + wantMappingNotification: false, + wantMapNotificationKeys: 0, }, - } - - repo, version := applyActionPinMapping("actions/checkout", "v4", ctx) - - assert.Equal(t, "acme-corp/checkout", repo, "repo should be replaced by mapping") - assert.Equal(t, "v4", version, "version should be replaced by mapping") -} - -func TestApplyActionPinMapping_OnlyMatchesExact(t *testing.T) { - ctx := &PinContext{ - Warnings: make(map[string]bool), - Mappings: map[string]string{ - "actions/checkout@v4": "acme-corp/checkout@v4", + { + name: "applies exact mapping", + actionRepo: "actions/checkout", + version: "v4", + mappings: map[string]string{ + "actions/checkout@v4": "acme-corp/checkout@v4", + }, + wantRepo: "acme-corp/checkout", + wantVersion: "v4", + wantMappingNotification: true, + wantMapNotificationKeys: 1, }, - } - - // Different version — should not match. - repo, version := applyActionPinMapping("actions/checkout", "v5", ctx) - - assert.Equal(t, "actions/checkout", repo, "repo should be unchanged when version does not match") - assert.Equal(t, "v5", version, "version should be unchanged when version does not match") -} - -func TestApplyActionPinMapping_DeduplicatesInfoMessage(t *testing.T) { - ctx := &PinContext{ - Warnings: make(map[string]bool), - Mappings: map[string]string{ - "actions/checkout@v4": "acme-corp/checkout@v4", + { + name: "does not match different version", + actionRepo: "actions/checkout", + version: "v5", + mappings: map[string]string{ + "actions/checkout@v4": "acme-corp/checkout@v4", + }, + wantRepo: "actions/checkout", + wantVersion: "v5", + wantMappingNotification: false, + wantMapNotificationKeys: 0, }, - } - - applyActionPinMapping("actions/checkout", "v4", ctx) - applyActionPinMapping("actions/checkout", "v4", ctx) - - notifyKey := "map:actions/checkout@v4" - assert.True(t, ctx.Warnings[notifyKey], "Expected notification key to be marked as seen") - // Count should be exactly one entry (deduplication). - mapNotifications := 0 - for k := range ctx.Warnings { - if strings.HasPrefix(k, "map:") { - mapNotifications++ - } - } - assert.Equal(t, 1, mapNotifications, "Expected exactly one deduplicated mapping notification") -} - -func TestApplyActionPinMapping_InvalidMappingValueSkipped(t *testing.T) { - ctx := &PinContext{ - Warnings: make(map[string]bool), - Mappings: map[string]string{ - "actions/checkout@v4": "no-at-sign", // invalid: missing at sign and version + { + name: "deduplicates notification for repeated mapping", + actionRepo: "actions/checkout", + version: "v4", + mappings: map[string]string{ + "actions/checkout@v4": "acme-corp/checkout@v4", + }, + repeat: 2, + wantRepo: "acme-corp/checkout", + wantVersion: "v4", + wantMappingNotification: true, + wantMapNotificationKeys: 1, + }, + { + name: "skips invalid mapping value", + actionRepo: "actions/checkout", + version: "v4", + mappings: map[string]string{ + "actions/checkout@v4": "no-at-sign", + }, + wantRepo: "actions/checkout", + wantVersion: "v4", + wantMappingNotification: false, + wantMapNotificationKeys: 0, + }, + { + name: "skips mapping target without ref separator", + actionRepo: "actions/checkout", + version: "v4", + mappings: map[string]string{ + "actions/checkout@v4": "acme-corp/checkout", + }, + wantRepo: "actions/checkout", + wantVersion: "v4", + wantMappingNotification: false, + wantMapNotificationKeys: 0, }, } - repo, version := applyActionPinMapping("actions/checkout", "v4", ctx) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := &PinContext{ + Warnings: make(map[string]bool), + Mappings: tt.mappings, + } + repeat := max(tt.repeat, 1) - // Invalid value should be silently skipped. - assert.Equal(t, "actions/checkout", repo, "repo should be unchanged for invalid mapping value") - assert.Equal(t, "v4", version, "version should be unchanged for invalid mapping value") -} + var gotRepo, gotVersion string + for range repeat { + gotRepo, gotVersion = applyActionPinMapping(tt.actionRepo, tt.version, ctx) + } -func TestApplyActionPinMapping_TargetWithoutRefSeparatorSkipped(t *testing.T) { - ctx := &PinContext{ - Warnings: make(map[string]bool), - Mappings: map[string]string{ - "actions/checkout@v4": "acme-corp/checkout", // invalid: missing @ separator - }, - } + assert.Equal(t, tt.wantRepo, gotRepo, "repo should match expected mapping outcome") + assert.Equal(t, tt.wantVersion, gotVersion, "version should match expected mapping outcome") - repo, version := applyActionPinMapping("actions/checkout", "v4", ctx) + notifyKey := "map:" + FormatCacheKey(tt.actionRepo, tt.version) + assert.Equal(t, tt.wantMappingNotification, ctx.Warnings[notifyKey], "mapping notification flag should match expected state") - assert.Equal(t, "actions/checkout", repo, "repo should be unchanged when mapping target has no @ separator") - assert.Equal(t, "v4", version, "version should be unchanged when mapping target has no @ separator") - assert.NotContains(t, ctx.Warnings, "map:actions/checkout@v4", "invalid mapping target should not set mapping warning key") + mapNotifications := 0 + for k := range ctx.Warnings { + if strings.HasPrefix(k, "map:") { + mapNotifications++ + } + } + assert.Equal(t, tt.wantMapNotificationKeys, mapNotifications, "unexpected number of mapping notification keys") + }) + } } diff --git a/pkg/actionpins/spec_test.go b/pkg/actionpins/spec_test.go index 9784acecc5d..8212909a906 100644 --- a/pkg/actionpins/spec_test.go +++ b/pkg/actionpins/spec_test.go @@ -1,5 +1,6 @@ //go:build !integration +// Package actionpins_test contains black-box specification tests for action pin resolution. package actionpins_test import (