test(actionpins): improve spec_test.go quality per testify-expert checklist

docs/adr/46470-adopt-table-driven-tests-in-actionpins-spec.md

@@ -0,0 +1,44 @@
+# ADR-46470: Adopt Table-Driven Tests as Canonical Pattern in actionpins Spec
+
+**Date**: 2026-07-18
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+`pkg/actionpins/spec_test.go` had grown with a mix of flat per-scenario test functions and table-driven tests. A testify-expert audit identified several quality problems: the `EnforcePinned` test table covered only failure paths, leaving the happy-path (resolver succeeds) untested; two separate flat functions (`TestSpec_PublicAPI_RecordResolutionFailure` and `TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed`) tested the same audit behaviour with different inputs, duplicating setup boilerplate; assertion style was inconsistent (`assert.Contains` mixed with `assert.Containsf`); and edge-cases for empty-SHA resolver fallthrough and `nil` `PinContext.Ctx` had no coverage at all. The decision was needed to define the standard test organisation pattern for this package so future contributors follow one consistent approach.
+
+### Decision
+
+We will adopt table-driven tests (`tests := []struct{...}`) as the canonical pattern for any test in `spec_test.go` that validates the same behaviour across multiple inputs or scenarios. Existing flat functions that duplicate setup for scenario variants will be merged into a single table-driven function. New edge-case tests (`TestSpec_DynamicResolution_EmptySHAFallsThrough`, `TestSpec_PublicAPI_ResolveActionPin_NilCtxField`) are added as standalone sub-test functions when they cover a clearly distinct code path rather than a parameter variant. All assertion calls will consistently use the `f`-suffix variants (`assert.Containsf`, `assert.Truef`) to ensure format strings carry the actual values.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep flat per-scenario test functions
+
+Each scenario gets its own top-level `TestSpec_*` function. This is the prior convention used for `TestSpec_PublicAPI_RecordResolutionFailure` and `TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed`. It is easy to name and find individual cases, but it duplicates setup code, makes it hard to see at a glance which cases are covered, and leads to drift when the shared logic changes (both functions must be updated separately).
+
+#### Alternative 2: Use `t.Run` subtests without a table struct
+
+Group related scenarios under a single parent test using named `t.Run` blocks, each with its own full setup. This avoids the flat-function duplication but does not enforce a shared struct schema, so field names and assertions can diverge between subtests. The table struct approach is strictly better when the inputs and expectations share the same shape, so this alternative is kept only for the genuinely distinct edge-case tests added in this PR.
+
+### Consequences
+
+#### Positive
+- Duplicate setup code is eliminated: the merged `TestSpec_PublicAPI_RecordResolutionFailure` function expresses both the no-resolver and failing-resolver scenarios from a single loop.
+- Coverage of `resolveActionPinDynamically` is now complete: the previously missing happy-path case (`resolver succeeds with EnforcePinned=true`) is captured in the existing `EnforcePinned` table, and the empty-SHA fallthrough is covered by the new test.
+- Assertion messages are richer: `f`-suffix variants print the actual values on failure, reducing the need to re-run tests with added logging.
+
+#### Negative
+- Table-driven tests make it slightly harder to isolate a single failing case during interactive debugging because the test loop runs all rows unless the developer uses `-run` with the subtest name.
+- Adding a new scenario now requires understanding the table struct shape, which has a mild learning-curve for contributors unfamiliar with the `wantResultSHA` / `wantErrorType` field conventions used here.
+
+#### Neutral
+- The two standalone flat tests that were merged cease to exist under their original names; `go test -run TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed` will no longer match — callers must use the table subtest path instead.
+- The new `TestSpec_DynamicResolution_EmptySHAFallsThrough` and `TestSpec_PublicAPI_ResolveActionPin_NilCtxField` are kept as standalone functions (not table rows) because their setup and assertions differ meaningfully from the existing tables.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/actionpins/spec_test.go

@@ -31,6 +31,7 @@ type testSHAResolver struct {
 	capturedRef  string
 }
 
+// ResolveSHA captures call arguments and returns the configured sha/err pair.
 func (r *testSHAResolver) ResolveSHA(ctx context.Context, repo, version string) (string, error) {
 	r.capturedCtx = ctx
 	r.capturedRepo = repo
@@ -270,6 +271,7 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {
 		allowActionRefs  bool
 		wantErr          bool
 		wantErrContains  string
+		wantResultSHA    string
 		wantFailureType  actionpins.ResolutionErrorType
 		wantFailureCount int
 		wantWarningKey   bool
@@ -296,6 +298,11 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {
 			wantFailureType:  actionpins.ResolutionErrorTypeDynamicResolutionFailed,
 			wantFailureCount: 1,
 		},
+		{
+			name:          "resolver succeeds with EnforcePinned=true returns pinned reference",
+			resolver:      &testSHAResolver{sha: testResolvedSHA},
+			wantResultSHA: testResolvedSHA,
+		},
 	}
 
 	for _, tt := range tests {
@@ -317,8 +324,15 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {
 				assert.Contains(t, err.Error(), tt.wantErrContains)
 				assert.Empty(t, result, "erroring enforce mode should not return a pinned reference")
 			} else {
-				require.NoError(t, err, "AllowActionRefs should downgrade unresolved pin enforcement to a warning")
-				assert.Empty(t, result, "downgraded unresolved result should remain empty")
+				require.NoError(t, err, "non-error scenario should not return an error")
+				if tt.wantResultSHA != "" {
+					assert.Equal(t,
+						actionpins.FormatPinnedActionReference("does-not-exist/x", tt.wantResultSHA, "v1"),
+						result,
+						"successful resolution should return the exact pinned reference format")
+				} else {
+					assert.Empty(t, result, "downgraded unresolved result should remain empty")
+				}
 			}
 
 			require.Len(t, failures, tt.wantFailureCount, "resolution failures should be audited consistently")
@@ -472,9 +486,9 @@ func TestSpec_DesignDecision_FormatConsistency(t *testing.T) {
 
 	assert.Truef(t, strings.HasPrefix(cacheKey, repo+"@"), "cache key should be repo@version, got %q", cacheKey)
 	assert.Truef(t, strings.HasPrefix(reference, repo+"@"), "reference should start with repo@sha, got %q", reference)
-	assert.Contains(t, cacheKey, version, "cache key should contain version")
-	assert.Contains(t, reference, sha, "reference should contain sha")
-	assert.Contains(t, reference, version, "reference should contain version comment")
+	assert.Containsf(t, cacheKey, version, "cache key should contain version %q", version)
+	assert.Containsf(t, reference, sha, "reference should contain sha %q", sha)
+	assert.Containsf(t, reference, version, "reference should contain version comment %q", version)
 }
 
 // TestSpec_Types_ActionPinsData validates the documented ActionPinsData container type.
@@ -571,8 +585,8 @@ func TestSpec_PublicAPI_GetContainerPin(t *testing.T) {
 		require.True(t, ok, "should return true for a known container image")
 		assert.Equal(t, knownImage, pin.Image, "ContainerPin.Image should match the queried image")
 		require.NotEmpty(t, pin.Digest, "ContainerPin.Digest should be non-empty for a known image")
-		assert.NotEmpty(t, pin.PinnedImage, "ContainerPin.PinnedImage should be non-empty for a known image")
-		assert.Contains(t, pin.PinnedImage, pin.Digest, "PinnedImage should contain the digest")
+		assert.True(t, strings.HasPrefix(pin.Digest, "sha256:"), "ContainerPin.Digest should use sha256: format, got %q", pin.Digest)
+		assert.Equal(t, knownImage+"@"+pin.Digest, pin.PinnedImage, "PinnedImage should equal image@digest")
 	})
 }
 
@@ -588,46 +602,52 @@ func TestSpec_Constants_ResolutionErrorType(t *testing.T) {
 
 // TestSpec_PublicAPI_RecordResolutionFailure validates the documented auditing behavior:
 // PinContext.RecordResolutionFailure collects ResolutionFailure events for unresolved pins,
-// classified with ResolutionErrorTypePinNotFound when no usable pin is found.
+// classified according to whether a resolver was present.
 // Spec section "Auditing Resolution Failures".
 func TestSpec_PublicAPI_RecordResolutionFailure(t *testing.T) {
-	var failures []actionpins.ResolutionFailure
-	ctx := &actionpins.PinContext{
-		Warnings: make(map[string]bool),
-		RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
-			failures = append(failures, f)
+	tests := []struct {
+		name          string
+		repo          string
+		version       string
+		resolver      actionpins.SHAResolver
+		wantErrorType actionpins.ResolutionErrorType
+	}{
+		{
+			name:          "no resolver classifies failure as pin_not_found",
+			repo:          "does-not-exist/unknown-action-xyzzy",
+			version:       "v1",
+			wantErrorType: actionpins.ResolutionErrorTypePinNotFound,
+		},
+		{
+			name:          "failing resolver classifies failure as dynamic_resolution_failed",
+			repo:          "does-not-exist/x",
+			version:       "v1",
+			resolver:      &testSHAResolver{err: errors.New("network error")},
+			wantErrorType: actionpins.ResolutionErrorTypeDynamicResolutionFailed,
 		},
 	}
 
-	_, err := actionpins.ResolveActionPin("does-not-exist/unknown-action-xyzzy", "v1", ctx)
-	require.NoError(t, err, "ResolveActionPin should not error even when the pin is unresolved")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var failures []actionpins.ResolutionFailure
+			ctx := &actionpins.PinContext{
+				Resolver: tt.resolver,
+				Warnings: make(map[string]bool),
+				RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
+					failures = append(failures, f)
+				},
+			}
 
-	require.Len(t, failures, 1, "RecordResolutionFailure should be invoked once for an unresolved pin")
-	assert.Equal(t, actionpins.ResolutionErrorTypePinNotFound, failures[0].ErrorType,
-		"unresolved pin with no resolver should be classified as pin_not_found")
-	assert.Equal(t, "does-not-exist/unknown-action-xyzzy", failures[0].Repo,
-		"recorded failure should carry the queried repo")
-	assert.Equal(t, "v1", failures[0].Ref, "recorded failure should carry the queried ref")
-}
+			_, err := actionpins.ResolveActionPin(tt.repo, tt.version, ctx)
+			require.NoError(t, err, "ResolveActionPin should not error for unresolved pins in non-enforce mode")
 
-// TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed validates dynamic-resolution failure auditing.
-func TestSpec_PublicAPI_RecordResolutionFailure_DynamicFailed(t *testing.T) {
-	var failures []actionpins.ResolutionFailure
-	ctx := &actionpins.PinContext{
-		Resolver: &testSHAResolver{err: errors.New("network error")},
-		Warnings: make(map[string]bool),
-		RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
-			failures = append(failures, f)
-		},
+			require.Len(t, failures, 1, "RecordResolutionFailure should be invoked once for an unresolved pin")
+			assert.Equal(t, tt.wantErrorType, failures[0].ErrorType,
+				"failure should be classified with the expected error type")
+			assert.Equal(t, tt.repo, failures[0].Repo, "recorded failure should carry the queried repo")
+			assert.Equal(t, tt.version, failures[0].Ref, "recorded failure should carry the queried ref")
+		})
 	}
-
-	_, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx)
-	require.NoError(t, err, "dynamic resolver failures should be audited and downgraded to unresolved pin")
-	require.Len(t, failures, 1, "expected one resolution failure to be recorded")
-	assert.Equal(t, actionpins.ResolutionErrorTypeDynamicResolutionFailed, failures[0].ErrorType,
-		"resolver error should classify as dynamic_resolution_failed")
-	assert.Equal(t, "does-not-exist/x", failures[0].Repo, "recorded failure should carry the queried repo")
-	assert.Equal(t, "v1", failures[0].Ref, "recorded failure should carry the queried ref")
 }
 
 // TestSpec_ThreadSafety_ConcurrentGetActionPinsByRepo validates that concurrent calls to GetActionPinsByRepo
@@ -677,6 +697,66 @@ func TestSpec_PublicAPI_ResolveActionPin_DynamicHappyPath(t *testing.T) {
 		"result should start with repo@sha in the documented format")
 }
 
+// TestSpec_DynamicResolution_EmptySHAFallsThrough validates that a resolver returning an empty
+// SHA with a nil error falls through to the hardcoded pin lookup rather than producing a result.
+func TestSpec_DynamicResolution_EmptySHAFallsThrough(t *testing.T) {
+	t.Run("empty SHA with nil error falls through to hardcoded pins for known repo", func(t *testing.T) {
+		known := "actions/checkout"
+		latestPin, ok := actionpins.GetLatestActionPinByRepo(known)
+		require.True(t, ok, "prerequisite: known repo must be in embedded data")
+
+		// Resolver returns ("", nil) — empty SHA is treated as a non-result and falls through.
+		ctx := &actionpins.PinContext{
+			Resolver: &testSHAResolver{sha: "", err: nil},
+			Warnings: make(map[string]bool),
+		}
+		result, err := actionpins.ResolveActionPin(known, latestPin.Version, ctx)
+		require.NoError(t, err)
+		require.NotEmpty(t, result, "empty-SHA resolver should fall through to hardcoded pins")
+		assert.Equal(t,
+			actionpins.FormatPinnedActionReference(known, latestPin.SHA, latestPin.Version),
+			result,
+			"result should match the exact hardcoded pin format")
+	})
+
+	t.Run("empty SHA with nil error falls through and produces empty for unknown repo", func(t *testing.T) {
+		// Resolver returns ("", nil) — empty SHA is treated as a non-result and falls through.
+		// The auditing contract requires a ResolutionFailure to be recorded for the unresolved pin.
+		var failures []actionpins.ResolutionFailure
+		ctx := &actionpins.PinContext{
+			Resolver: &testSHAResolver{sha: "", err: nil},
+			Warnings: make(map[string]bool),
+			RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
+				failures = append(failures, f)
+			},
+		}
+		result, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx)
+		require.NoError(t, err)
+		assert.Empty(t, result, "empty-SHA resolver on unknown repo should produce empty result")
+		require.Len(t, failures, 1, "unresolved pin should be recorded as a resolution failure")
+		assert.Equal(t, actionpins.ResolutionErrorTypeDynamicResolutionFailed, failures[0].ErrorType,
+			"failure should be classified as dynamic resolution failed")
+	})
+}
+
+// TestSpec_PublicAPI_ResolveActionPin_NilCtxField validates that a nil PinContext.Ctx
+// falls back to context.Background() instead of panicking.
+func TestSpec_PublicAPI_ResolveActionPin_NilCtxField(t *testing.T) {
+	resolver := &testSHAResolver{sha: testResolvedSHA}
+	ctx := &actionpins.PinContext{
+		Ctx:      nil, // deliberately nil — should fall back to context.Background()
+		Resolver: resolver,
+		Warnings: make(map[string]bool),
+	}
+	require.NotPanics(t, func() {
+		result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
+		assert.NoError(t, err)
+		assert.NotEmpty(t, result)
+	}, "nil PinContext.Ctx should fall back to context.Background() without panicking")
+	require.NotNil(t, resolver.capturedCtx, "resolver must receive a non-nil context even when PinContext.Ctx is nil")
+	assert.Equal(t, context.Background(), resolver.capturedCtx, "resolver must receive context.Background() as the documented fallback")
+}
+
 // TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext validates that PinContext.Ctx
 // is forwarded to the resolver instead of being replaced with context.Background().
 func TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext(t *testing.T) {