[testify-expert] Improve Test Quality: pkg/parser/frontmatter_utils_test.go

[github-actions]

The test file pkg/parser/frontmatter_utils_test.go has been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability.

Current State

• Test File: pkg/parser/frontmatter_utils_test.go
• Source Files: pkg/parser/remote_fetch.go, pkg/parser/include_processor.go
• Test Functions: 8 test functions
• Lines of Code: 818 lines

Test Quality Analysis

Strengths ✅

1. Consistent testify usage — require.* for setup assertions, assert.* for validations, with helpful contextual messages throughout (e.g., assert.Equal(t, tt.expected, result, "isUnderWorkflowsDirectory(%q)", tt.filePath))
2. Comprehensive table-driven tests — All 8 test functions use t.Run() with descriptive names, including well-documented edge cases (security traversal attacks, .github-repo-named repositories, all path styles)
3. Good security coverage — TestResolveIncludePath_AllPathStyles covers path traversal attacks like ../../etc/passwd in all prefix variants

🎯 Areas for Improvement

1. Missing Tests for isNotFoundError

Location: pkg/parser/remote_fetch.go:609

This is a pure, side-effect-free function with no network dependencies and trivial logic — a perfect unit test candidate that is currently untested.

// isNotFoundError checks if an error message indicates a 404 Not Found response
func isNotFoundError(errMsg string) bool {
    lowerMsg := strings.ToLower(errMsg)
    return strings.Contains(lowerMsg, "404") || strings.Contains(lowerMsg, "not found")
}

Recommended Test:

func TestIsNotFoundError(t *testing.T) {
    tests := []struct {
        name     string
        errMsg   string
        expected bool
    }{
        {
            name:     "message containing 404",
            errMsg:   "HTTP 404: resource not accessible",
            expected: true,
        },
        {
            name:     "message containing 'not found' (lowercase)",
            errMsg:   "not found",
            expected: true,
        },
        {
            name:     "message containing 'Not Found' (mixed case)",
            errMsg:   "404 Not Found",
            expected: true,
        },
        {
            name:     "unrelated error message",
            errMsg:   "connection refused",
            expected: false,
        },
        {
            name:     "empty string",
            errMsg:   "",
            expected: false,
        },
        {
            name:     "message with 4040 (not a 404)",
            errMsg:   "error code 4040",
            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)
        })
    }
}

2. Missing Tests for frontmatterContainsExpressions and containsExpression

Location: pkg/parser/include_processor.go:286-310

These are pure recursive functions used to defer schema validation when import-schema expressions are present. They have no external dependencies and should be unit tested.

func frontmatterContainsExpressions(m map[string]any) bool { ... }
func containsExpression(v any) bool { ... }

Recommended Tests:

func TestFrontmatterContainsExpressions(t *testing.T) {
    tests := []struct {
        name     string
        input    map[string]any
        expected bool
    }{
        {
            name:     "empty map",
            input:    map[string]any{},
            expected: false,
        },
        {
            name:     "plain string values, no expressions",
            input:    map[string]any{"engine": "copilot", "version": "1.0"},
            expected: false,
        },
        {
            name:     "string value with expression",
            input:    map[string]any{"tools": "$\{\{ github.aw.import-inputs.tools }}"},
            expected: true,
        },
        {
            name:     "nested map with expression",
            input:    map[string]any{
                "serena": map[string]any{
                    "path": "$\{\{ github.aw.import-inputs.path }}",
                },
            },
            expected: true,
        },
        {
            name:     "slice with expression",
            input:    map[string]any{
                "allowed": []any{"ls", "$\{\{ github.aw.import-inputs.cmd }}"},
            },
            expected: true,
        },
        {
            name:     "nested map with no expressions",
            input:    map[string]any{
                "tools": map[string]any{"bash": map[string]any{"allowed": []any{"ls"}}},
            },
            expected: false,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result := frontmatterContainsExpressions(tt.input)
            assert.Equal(t, tt.expected, result, "frontmatterContainsExpressions(%v)", tt.input)
        })
    }
}

3. TestIsWorkflowSpec Tests the Unexported Wrapper Instead of the Exported Function

Location: pkg/parser/remote_fetch.go:205-260

The test file calls the unexported isWorkflowSpec (line 255 in source is just return IsWorkflowSpec(path)), but the exported IsWorkflowSpec is the documented API with the full logic. The test should directly test the exported function and its documented contract.

// Current test (tests the thin wrapper)
got := isWorkflowSpec(tt.path)

// Improved (tests the actual exported API)
got := IsWorkflowSpec(tt.path)

This also adds a notable gap: the current test does not cover the "://" URL-like path case which the exported function handles specially:

// From IsWorkflowSpec — currently not tested
if strings.Contains(cleanPath, "://") {
    return true  // <- this branch is untested!
}

Add this case to the table:

{
    name: "URL-like path with :// is treated as workflowspec",
    path: "(example.com/redacted),
    want: true,
},

4. Duplicated Filesystem Setup Across Three Test Functions

Location: TestResolveIncludePath, TestResolveIncludePath_DotGithubRepo, TestResolveIncludePath_AllPathStyles

Each test independently creates a near-identical directory tree (.github/workflows/, .github/agents/, .agents/) with test files. This setup is roughly 30 lines repeated three times.

Recommended refactor: Extract a shared helper:

type repoLayout struct {
    repoRoot     string
    workflowsDir string
    agentsDir    string
    dotAgentsDir string
    workflowFile string
    agentFile    string
    dotAgentFile string
}

func createTestRepoLayout(t *testing.T, tempDir string) repoLayout {
    t.Helper()
    repoRoot := filepath.Join(tempDir, "repo")
    workflowsDir := filepath.Join(repoRoot, ".github", "workflows")
    agentsDir := filepath.Join(repoRoot, ".github", "agents")
    dotAgentsDir := filepath.Join(repoRoot, ".agents")

    for _, dir := range []string{workflowsDir, agentsDir, dotAgentsDir} {
        require.NoError(t, os.MkdirAll(dir, 0755), "should create dir %s", dir)
    }

    workflowFile := filepath.Join(workflowsDir, "workflow.md")
    agentFile := filepath.Join(agentsDir, "planner.md")
    dotAgentFile := filepath.Join(dotAgentsDir, "agent.md")

    for path, content := range map[string]string{
        workflowFile: "workflow",
        agentFile:    "planner",
        dotAgentFile: "dot-agent",
    } {
        require.NoError(t, os.WriteFile(path, []byte(content), 0644), "should write %s", path)
    }

    return repoLayout{repoRoot, workflowsDir, agentsDir, dotAgentsDir, workflowFile, agentFile, dotAgentFile}
}

Why this matters: Reduces duplication, makes tests easier to maintain, and ensures all tests use consistent fixture data.

5. Incomplete TestProcessImportsFromFrontmatter Coverage

The current test has only 4 cases. Important scenarios not tested:

• Imports with engines — the wantEngines: true case is never exercised
• Multiple valid imports — only single import tested
• Imports from non-existent file — should return an error

Add these test cases:

{
    name: "import file with engines field",
    frontmatter: map[string]any{
        "on": "push",
        "imports": []string{"engine-include.md"}, // a file that specifies engine:
    },
    wantToolsJSON: false,
    wantEngines:   true,
    wantErr:       false,
},
{
    name: "import file that does not exist",
    frontmatter: map[string]any{
        "on":      "push",
        "imports": []string{"does-not-exist.md"},
    },
    wantErr: true,
},

📋 Implementation Guidelines

Priority Order

1. High: Add TestIsNotFoundError — pure function, trivially testable, no setup required
2. High: Switch TestIsWorkflowSpec to test IsWorkflowSpec (exported) and add the :// branch coverage
3. Medium: Add TestFrontmatterContainsExpressions — pure recursive function, good coverage value
4. Medium: Expand TestProcessImportsFromFrontmatter with the engines and missing-file cases
5. Low: Extract createTestRepoLayout helper to reduce duplication

Best Practices from scratchpad/testing.md

• ✅ Use require.* for critical setup (stops test on failure)
• ✅ Use assert.* for test validations (continues checking)
• ✅ Write table-driven tests with t.Run() and descriptive names
• ✅ No mocks or test suites - test real component interactions
• ✅ Always include helpful assertion messages

Testing Commands

# Run tests for this package
go test -v ./pkg/parser/ -run TestIsNotFoundError
go test -v ./pkg/parser/ -run TestFrontmatterContainsExpressions
go test -v ./pkg/parser/ -run TestIsWorkflowSpec

# Run all parser unit tests
go test ./pkg/parser/

# Full validation
make test-unit

Acceptance Criteria

• TestIsNotFoundError added with at least 5 table-driven cases (truthy 404 strings, falsy strings, edge cases)
• TestFrontmatterContainsExpressions added covering: empty map, flat expressions, nested map, slice, no expressions
• TestIsWorkflowSpec updated to call exported IsWorkflowSpec and covers the :// branch
• TestProcessImportsFromFrontmatter expanded with engines case and missing-file error case
• (Optional) createTestRepoLayout helper extracted to reduce filesystem setup duplication
• All tests pass: go test ./pkg/parser/
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Source of isNotFoundError: pkg/parser/remote_fetch.go:609
• Source of frontmatterContainsExpressions: pkg/parser/include_processor.go:286
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (most improvements are adding new table cases or small test functions)
Expected Impact: Improved coverage of pure utility functions, catches regressions in isNotFoundError logic, ensures exported API IsWorkflowSpec is contract-tested

Files Involved:

• Test file: pkg/parser/frontmatter_utils_test.go
• Source files: pkg/parser/remote_fetch.go, pkg/parser/include_processor.go

References:

• Workflow Run §24664720085

Generated by Daily Testify Uber Super Expert · ● 1.5M · ◷

• expires on Apr 22, 2026, 11:53 AM UTC