Improve parser utility test coverage for not-found detection, expression scanning, workflow spec classification, and import edge cases

docs/adr/27327-parser-utility-test-strategy.md

@@ -0,0 +1,74 @@
+# ADR-27327: Test Strategy for Parser Package Utility Functions
+
+**Date**: 2026-04-20
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The `pkg/parser` package contains several utility functions that underpin frontmatter processing: `IsWorkflowSpec`, `isNotFoundError`, `frontmatterContainsExpressions`, and `processImportsFromFrontmatter`. These functions handle critical path logic for workflow file parsing, error classification, and import processing. Prior to this change, test coverage for these utilities was incomplete: existing tests called unexported helpers (e.g., `isWorkflowSpec`) rather than the exported API, error-path branches for `processImportsFromFrontmatter` were untested, and no dedicated coverage existed for `isNotFoundError` or `frontmatterContainsExpressions`. A test quality review surfaced these gaps and motivated a systematic coverage improvement targeting both correctness and long-term maintainability.
+
+### Decision
+
+We will test parser utility functions through their exported API surface (e.g., `IsWorkflowSpec` rather than the unexported `isWorkflowSpec`) and will provide comprehensive table-driven tests that cover empty inputs, truthy and falsy edge cases, and near-miss inputs for each utility predicate. For `processImportsFromFrontmatter`, we will explicitly test the engine-metadata propagation path and the missing-file error path, asserting on a specific error message substring via `require.ErrorContains`. Test function names within the package must be unique to avoid silent shadowing across files.
+
+### Alternatives Considered
+
+#### Alternative 1: Test Private Helpers Directly
+
+Unexported functions such as `isWorkflowSpec` are accessible within the same package and could be tested directly without exporting them. This was the prior approach. It was rejected because it couples tests to internal naming; if the function is renamed, moved, or inlined, tests break without any change to observable public behavior. Testing through the exported API (`IsWorkflowSpec`) means tests survive internal refactoring and document the intended public contract.
+
+#### Alternative 2: Integration-Level Tests Only
+
+Parser utilities could be tested exclusively at the workflow-loading integration level — parsing a complete workflow file and asserting the final result. This was not chosen because integration tests are slower to execute, harder to debug on failure, and do not isolate which utility function is at fault. Focused unit-level table-driven tests provide faster feedback and pinpoint failures precisely.
+
+### Consequences
+
+#### Positive
+- Tests are decoupled from private implementation details and survive internal refactoring without modification.
+- Table-driven test structure makes adding new edge-case inputs trivial without writing new test functions.
+- `require.ErrorContains` assertions on error paths produce actionable failure messages that name the expected error substring.
+- Removing the duplicate `TestIsNotFoundError` name across test files prevents silent test shadowing detectable only by the Go toolchain.
+
+#### Negative
+- Functions such as `isNotFoundError` and `frontmatterContainsExpressions` remain unexported; tests in `pkg/parser` that exercise them cannot be moved to a black-box `parser_test` package without promoting those functions to exported, creating mild coupling to the package's internal API.
+- The engine-content fixture file written to `tempDir` in `TestProcessImportsFromFrontmatter` adds setup complexity; future test cases in the same function must account for that fixture's presence.
+
+#### Neutral
+- The renamed function `TestIsNotFoundError_RemoteNested` in `import_remote_nested_test.go` disambiguates the two test functions but changes the test name string reported in CI output and `go test -v` listings.
+- No production code is modified by this decision; all changes are confined to `_test.go` files.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### API Surface Testing
+
+1. Tests for functions with both an exported and an unexported variant **MUST** call the exported variant (e.g., `IsWorkflowSpec` rather than `isWorkflowSpec`).
+2. Tests **MUST NOT** call unexported functions when an exported equivalent exists or when the behavior is reachable through a higher-level exported function.
+3. When an unexported function is promoted to exported, tests **SHOULD** be updated to call the exported variant in the same PR that makes the promotion.
+
+### Table-Driven Test Structure
+
+1. Tests for utility predicates and classifiers (e.g., `isNotFoundError`, `frontmatterContainsExpressions`) **MUST** use Go table-driven tests (`[]struct{ ... }` with `t.Run`).
+2. Each table **MUST** include at minimum: an empty or zero-value input, at least one input that produces a `true`/non-nil result, and at least one near-miss input that is superficially similar to a truthy case but should produce a `false`/nil result.
+3. Error-path test cases **SHOULD** assert on a specific error message substring using `require.ErrorContains` rather than asserting only that a non-nil error was returned.
+
+### Test Function Naming
+
+1. Test function names within a Go package **MUST** be globally unique across all `_test.go` files in that package.
+2. When a naming collision is resolved by renaming a test function, the new name **SHOULD** include a suffix that identifies the originating file or scenario domain (e.g., `_RemoteNested`).
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/parser/frontmatter_utils_test.go

@@ -612,12 +612,17 @@ func TestIsWorkflowSpec(t *testing.T) {
 			path: "owner//path/file.md",
 			want: false,
 		},
+		{
+			name: "URL-like path with scheme",
+			path: "https://github.com/owner/repo/path/to/file.md",
+			want: true,
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := isWorkflowSpec(tt.path)
-			assert.Equal(t, tt.want, got, "isWorkflowSpec(%q)", tt.path)
+			got := IsWorkflowSpec(tt.path)
+			assert.Equal(t, tt.want, got, "IsWorkflowSpec(%q)", tt.path)
 		})
 	}
 }
@@ -736,12 +741,24 @@ This is an included file.`
 	err := os.WriteFile(includeFile, []byte(includeContent), 0644)
 	require.NoError(t, err, "should write include file")
 
+	// Create a test include file with an engine definition
+	engineFile := filepath.Join(tempDir, "engine.md")
+	engineContent := `---
+engine: copilot
+---
+# Engine Include
+This include defines an engine.`
+	err = os.WriteFile(engineFile, []byte(engineContent), 0644)
+	require.NoError(t, err, "should write engine file")
+
 	tests := []struct {
 		name          string
 		frontmatter   map[string]any
 		wantToolsJSON bool
 		wantEngines   bool
+		wantEngineHas string
 		wantErr       bool
+		wantErrHas    string
 	}{
 		{
 			name: "no imports field",
@@ -782,14 +799,39 @@ This is an included file.`
 			wantEngines:   false,
 			wantErr:       true,
 		},
+		{
+			name: "valid imports with engine",
+			frontmatter: map[string]any{
+				"on":      "push",
+				"imports": []string{"engine.md"},
+			},
+			wantToolsJSON: true,
+			wantEngines:   true,
+			wantEngineHas: "copilot",
+			wantErr:       false,
+		},
+		{
+			name: "missing import file",
+			frontmatter: map[string]any{
+				"on":      "push",
+				"imports": []string{"missing.md"},
+			},
+			wantToolsJSON: false,
+			wantEngines:   false,
+			wantErr:       true,
+			wantErrHas:    "file not found",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			tools, engines, err := processImportsFromFrontmatter(tt.frontmatter, tempDir)
 
 			if tt.wantErr {
-				assert.Error(t, err, "ProcessImportsFromFrontmatter() should return error")
+				require.Error(t, err, "ProcessImportsFromFrontmatter() should return error")
+				if tt.wantErrHas != "" {
+					require.ErrorContains(t, err, tt.wantErrHas, "ProcessImportsFromFrontmatter() error should contain %q", tt.wantErrHas)
+				}
 				return
 			}
 
@@ -807,6 +849,9 @@ This is an included file.`
 
 			if tt.wantEngines {
 				assert.NotEmpty(t, engines, "ProcessImportsFromFrontmatter() should return engines")
+				if tt.wantEngineHas != "" {
+					assert.Contains(t, engines[0], tt.wantEngineHas, "ProcessImportsFromFrontmatter() engine should contain expected value")
+				}
 			} else {
 				assert.Empty(t, engines, "ProcessImportsFromFrontmatter() should return no engines")
 			}
@@ -816,3 +861,109 @@ This is an included file.`
 
 // TestProcessIncludedFileWithNameAndDescription verifies that name and description fields
 // do not generate warnings when processing included files outside .github/workflows/
+func TestIsNotFoundError(t *testing.T) {
+	tests := []struct {
+		name     string
+		errMsg   string
+		expected bool
+	}{
+		{
+			name:     "HTTP 404 text",
+			errMsg:   "HTTP 404: Not Found",
+			expected: true,
+		},
+		{
+			name:     "not found phrase",
+			errMsg:   "failed to fetch file: not found",
+			expected: true,
+		},
+		{
+			name:     "uppercase not found phrase",
+			errMsg:   "RESOURCE NOT FOUND",
+			expected: true,
+		},
+		{
+			name:     "non-404 status",
+			errMsg:   "HTTP 401: Unauthorized",
+			expected: false,
+		},
+		{
+			name:     "word without space",
+			errMsg:   "remote returned notfound response",
+			expected: false,
+		},
+		{
+			name:     "empty string",
+			errMsg:   "",
+			expected: false,
+		},
+		{
+			name:     "whitespace-only message",
+			errMsg:   "   ",
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isNotFoundError(tt.errMsg)
+			assert.Equal(t, tt.expected, result, "isNotFoundError(%q)", tt.errMsg)
+		})
+	}
+}
+
+func TestFrontmatterContainsExpressions(t *testing.T) {
+	tests := []struct {
+		name        string
+		frontmatter map[string]any
+		expected    bool
+	}{
+		{
+			name:        "empty map",
+			frontmatter: map[string]any{},
+			expected:    false,
+		},
+		{
+			name: "flat expression",
+			frontmatter: map[string]any{
+				"name": "${{ github.actor }}",
+			},
+			expected: true,
+		},
+		{
+			name: "nested map expression",
+			frontmatter: map[string]any{
+				"tools": map[string]any{
+					"server": "${{ github.aw.import-inputs.server }}",
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "slice expression",
+			frontmatter: map[string]any{
+				"labels": []any{"triage", "${{ github.ref_name }}"},
+			},
+			expected: true,
+		},
+		{
+			name: "no expressions",
+			frontmatter: map[string]any{
+				"name": "workflow",
+				"tools": map[string]any{
+					"bash": map[string]any{
+						"allowed": []any{"ls", "cat"},
+					},
+				},
+			},
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := frontmatterContainsExpressions(tt.frontmatter)
+			assert.Equal(t, tt.expected, result, "frontmatterContainsExpressions(%v)", tt.frontmatter)
+		})
+	}
+}

pkg/parser/import_remote_nested_test.go

@@ -519,7 +519,7 @@ func TestImportQueueItemRemoteOriginField(t *testing.T) {
 	})
 }
 
-func TestIsNotFoundError(t *testing.T) {
+func TestIsNotFoundError_RemoteNested(t *testing.T) {
 	tests := []struct {
 		name     string
 		errMsg   string