[actionpins] Add missing internal coverage and table-drive action pin mapping tests

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.*

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")
+		})
+	}
 }

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 (