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

[github-actions]

Overview

The test file pkg/parser/engine_includes_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 using testify best practices.

Current State

• Test File: pkg/parser/engine_includes_test.go
• Source Functions Tested: ExpandIncludesForEngines, extractFrontmatterField
• Test Functions: 7 test functions
• Lines of Code: 342 lines

Test Quality Analysis

Strengths ✅

1. Good scenario coverage — Tests cover string engines, object engines, no engine, multiple includes, optional missing includes, and command-based engines, which represents solid functional breadth.
2. Uses t.Run() in TestExtractEngineFromContent — That one test already uses table-driven structure correctly.
3. Descriptive test names — Function names clearly describe the scenario being tested (e.g., TestExpandIncludesForEnginesObjectFormat).

🎯 Areas for Improvement

1. No Testify Assertions — Uses Manual Error Checking Throughout

The entire file uses manual if err != nil { t.Fatal(...) } and if x != y { t.Errorf(...) } instead of testify assertions. The package has no testify import at all.

Current Issues:

// ❌ CURRENT (anti-patterns throughout the file)
if err := os.WriteFile(includeFile, []byte(includeContent), 0644); err != nil {
    t.Fatal(err)
}
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
    t.Fatalf("Expected successful engine expansion, got error: %v", err)
}
if len(engines) != 1 {
    t.Fatalf("Expected 1 engine, got %d", len(engines))
}
if engines[0] != `"codex"` {
    t.Errorf("Expected engine 'codex', got %s", engines[0])
}

Recommended Changes:

// ✅ IMPROVED (testify assertions)
import (
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

err := os.WriteFile(includeFile, []byte(includeContent), 0644)
require.NoError(t, err, "should write include file")

engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
require.NoError(t, err, "ExpandIncludesForEngines should succeed")
require.Len(t, engines, 1, "should find exactly one engine")
assert.Equal(t, `"codex"`, engines[0], "should extract codex engine")

Why this matters: Testify provides clearer error messages, better test output, and is the standard across this codebase (see scratchpad/testing.md).

2. Six Similar Tests Should Be Table-Driven

TestExpandIncludesForEngines, TestExpandIncludesForEnginesObjectFormat, TestExpandIncludesForEnginesNoEngine, TestExpandIncludesForEnginesMultipleIncludes, TestExpandIncludesForEnginesOptionalMissing, and TestExpandIncludesForEnginesWithCommand all follow the exact same structure: create tmpDir, write include file(s), build mainContent, call ExpandIncludesForEngines, assert. They should be merged into one table-driven test.

Recommended Changes:

func TestExpandIncludesForEngines(t *testing.T) {
    tests := []struct {
        name           string
        includeFiles   map[string]string // filename -> content
        mainContent    string
        expectedCount  int
        expectedEngine string   // for single-engine cases
        checkFields    []string // for object-format cases
    }{
        {
            name: "string engine",
            includeFiles: map[string]string{
                "include-engine.md": "---\nengine: codex\n---\n",
            },
            mainContent:    "`@include` include-engine.md\n",
            expectedCount:  1,
            expectedEngine: `"codex"`,
        },
        {
            name: "object format engine",
            includeFiles: map[string]string{
                "include-object.md": "---\nengine:\n  id: claude\n  model: claude-3-5-sonnet-20241022\n  max-turns: 5\n---\n",
            },
            mainContent:   "`@include` include-object.md\n",
            expectedCount: 1,
            checkFields:   []string{`"id":"claude"`, `"model":"claude-3-5-sonnet-20241022"`, `"max-turns":5`},
        },
        {
            name: "no engine in include",
            includeFiles: map[string]string{
                "include-no-engine.md": "---\ntools:\n  github: {}\n---\n",
            },
            mainContent:   "`@include` include-no-engine.md\n",
            expectedCount: 0,
        },
        {
            name: "optional missing include",
            mainContent:   "`@include`? missing-file.md\n",
            expectedCount: 0,
        },
        {
            name: "multiple includes",
            includeFiles: map[string]string{
                "include1.md": "---\nengine: claude\n---\n",
                "include2.md": "---\nengine: codex\n---\n",
            },
            mainContent:   "`@include` include1.md\n@include include2.md\n",
            expectedCount: 2,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            tmpDir := testutil.TempDir(t, "test-*")
            for name, content := range tt.includeFiles {
                err := os.WriteFile(filepath.Join(tmpDir, name), []byte(content), 0644)
                require.NoError(t, err, "should write include file %s", name)
            }

            engines, err := ExpandIncludesForEngines(tt.mainContent, tmpDir)
            require.NoError(t, err, "ExpandIncludesForEngines should succeed")
            require.Len(t, engines, tt.expectedCount, "should find expected number of engines")

            if tt.expectedEngine != "" {
                assert.Equal(t, tt.expectedEngine, engines[0], "first engine should match")
            }
            for _, field := range tt.checkFields {
                assert.Contains(t, engines[0], field, "engine JSON should contain field")
            }
        })
    }
}

Why this matters: Reduces 6 x ~30-line functions down to one table-driven test. New scenarios require only a new struct entry, not a new function.

3. TestExtractEngineFromContent Partially Done — Needs Testify

The one table-driven test in the file (TestExtractEngineFromContent) still uses manual error checks inside t.Run():

// ❌ CURRENT
result, err := extractFrontmatterField(tt.content, "engine", "")
if err != nil {
    t.Fatalf("Unexpected error: %v", err)
}
if result != tt.expected {
    t.Errorf("Expected %q, got %q", tt.expected, result)
}

// ✅ IMPROVED
result, err := extractFrontmatterField(tt.content, "engine", "")
require.NoError(t, err, "extractFrontmatterField should not error")
assert.Equal(t, tt.expected, result, "extracted engine should match expected")

4. Missing Assertion Messages

Many assertions lack messages, making failures harder to debug. All assert.* and require.* calls should include a descriptive message as the final argument.

5. Missing Edge Case Tests for ExpandIncludesForEngines

Consider adding:

• Nested includes: An include file that itself has @include directives
• Malformed frontmatter: An include with invalid YAML — should it error or skip?
• Required missing include: @include non-existent.md (no ?) — should return an error

📋 Implementation Guidelines

Priority Order

1. High: Add testify import and convert all manual error checks
2. High: Merge the 6 TestExpandIncludesForEngines* functions into one table-driven test
3. Medium: Add missing assertion messages
4. Low: Add edge case tests for malformed/nested includes

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 this specific test file
go test -v ./pkg/parser/ -run "TestExpandIncludesForEngines|TestExtractEngine"

# Run all parser tests
go test -v ./pkg/parser/

# Run all unit tests
make test-unit

Acceptance Criteria

• Add github.com/stretchr/testify/assert and require imports
• Convert all t.Fatal / t.Fatalf calls to require.NoError / require.Len / etc.
• Convert all t.Errorf calls to assert.Equal / assert.Contains / etc.
• Merge the 6 similar TestExpandIncludesForEngines* functions into one table-driven test
• Add descriptive messages to all assertions
• Tests pass: go test -v ./pkg/parser/ -run TestExpandIncludesForEngines
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Related test for reference: pkg/parser/content_extractor_features_test.go (same package, also needs testify)
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (mostly mechanical substitution + one table-driven refactor)
Expected Impact: Cleaner test output on failure, easier to add new engine scenarios, consistent style with rest of codebase

Files Involved:

• Test file: pkg/parser/engine_includes_test.go
• Source file: pkg/parser/include_expander.go / pkg/parser/content_extractor.go

Generated by 🧪 Daily Testify Uber Super Expert · ● 14.4M · ◷

• expires on May 20, 2026, 6:32 PM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ● 114.8K · ◷