Skip to content

Refactor parser engine-include tests to table-driven testify assertions #33241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
May 19, 2026
+113 −208
Merged

Refactor parser engine-include tests to table-driven testify assertions #33241

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd09239
Initial plan
Copilot May 19, 2026
a0a2e8e
test(parser): refactor engine includes tests to testify table-driven …
Copilot May 19, 2026
d68adc8
test(parser): refine assertion message format in engine includes tabl…
Copilot May 19, 2026
377e6da
test(parser): make engine include expectations explicit in table cases
Copilot May 19, 2026
a1a78bf
test(parser): use assert.Containsf for descriptive field checks
Copilot May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
321 changes: 113 additions & 208 deletions pkg/parser/engine_includes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,58 +8,45 @@ import (
"testing"

"github.com/github/gh-aw/pkg/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestExpandIncludesForEngines(t *testing.T) {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The table structure handles success cases well, but there's no field for expected errors. Consider adding negative test cases for:

  • Malformed engine YAML (e.g., engine: [invalid, list])
  • Invalid JSON in object engines
  • Circular include references
  • File read errors (beyond missing optional includes)

Add an expectedErr string field and a few failure scenarios:

{
    name: "malformed engine yaml",
    includeFiles: map[string]string{
        "bad.md": `---\nengine: [1, 2, 3]\n---`,
    },
    mainContent: "`@include` bad.md",
    expectedErr: "invalid engine format",
}

Testing error paths prevents regressions when error handling changes.

// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")

// Create include file with engine specification
includeContent := `---
tests := []struct {
name string
includeFiles map[string]string
mainContent string
expectedLen int
expectedExact map[int]string
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The expectedExact and expectedContains maps use int keys for indices, but the test cases only validate index 0 or 1. This design assumes engines are returned in include order, but that's not explicitly documented or guaranteed.

Consider either:

  1. Add a comment documenting the ordering guarantee:
// expectedExact validates engines by index (engines returned in include directive order)
expectedExact map[int]string
  1. Or make it order-independent by validating the set of engines instead of positional:
expectedEngines []string  // Any order
for _, expected := range tt.expectedEngines {
    assert.Contains(t, engines, expected)
}

Option 2 is more robust if include processing order ever changes.

expectedContains map[int][]string
}{
{
name: "single string engine",
includeFiles: map[string]string{
"include-engine.md": `---
engine: codex
tools:
github:
allowed: ["list_issues"]
---

# Include with Engine
`
includeFile := filepath.Join(tmpDir, "include-engine.md")
if err := os.WriteFile(includeFile, []byte(includeContent), 0644); err != nil {
t.Fatal(err)
}

// Create main markdown content with include directive
mainContent := `# Main Workflow
`,
},
mainContent: `# Main Workflow

@include include-engine.md

Some content here.
`

// Test engine expansion
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion, got error: %v", err)
}

// Should find one engine
if len(engines) != 1 {
t.Fatalf("Expected 1 engine, got %d", len(engines))
}

// Should extract "codex" engine
if engines[0] != `"codex"` {
t.Errorf("Expected engine 'codex', got %s", engines[0])
}
}

func TestExpandIncludesForEnginesObjectFormat(t *testing.T) {
// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")

// Create include file with object-format engine specification
includeContent := `---
`,
expectedLen: 1,
expectedExact: map[int]string{0: `"codex"`},
},
{
name: "object format engine",
includeFiles: map[string]string{
"include-object-engine.md": `---
engine:
id: claude
model: claude-3-5-sonnet-20241022
Expand All @@ -70,114 +57,62 @@ tools:
---

# Include with Object Engine
`
includeFile := filepath.Join(tmpDir, "include-object-engine.md")
if err := os.WriteFile(includeFile, []byte(includeContent), 0644); err != nil {
t.Fatal(err)
}

// Create main markdown content with include directive
mainContent := `# Main Workflow
`,
},
mainContent: `# Main Workflow

@include include-object-engine.md

Some content here.
`

// Test engine expansion
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion, got error: %v", err)
}

// Should find one engine
if len(engines) != 1 {
t.Fatalf("Expected 1 engine, got %d", len(engines))
}

// Should extract engine object as JSON
expectedFields := []string{`"id":"claude"`, `"model":"claude-3-5-sonnet-20241022"`, `"max-turns":5`}
for _, field := range expectedFields {
if !contains(engines[0], field) {
t.Errorf("Expected engine JSON to contain %s, got %s", field, engines[0])
}
}
}

func TestExpandIncludesForEnginesNoEngine(t *testing.T) {
// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")

// Create include file without engine specification
includeContent := `---
`,
expectedLen: 1,
expectedContains: map[int][]string{
0: {`"id":"claude"`, `"model":"claude-3-5-sonnet-20241022"`, `"max-turns":5`},
},
},
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] require.NoError is used here for the function-under-test, but this should use assert.NoError instead. Follow the pattern:

  • require.* — Setup that must succeed (file writes, temp dirs)
  • assert.* — Actual test validations (the SUT behavior)

This line tests ExpandIncludesForEngines itself, so it's a validation, not setup:

assert.NoError(t, err, "Should expand includes for test case %q", tt.name)

Using require here stops the entire test suite on first failure rather than collecting all failing scenarios.

{
name: "include without engine",
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The error message includes %q for tt.name, but test failures already include the subtest name from t.Run(tt.name, ...). This creates redundant output:

--- FAIL: TestExpandIncludesForEngines/single_string_engine
    Should expand includes for test case "single string engine"

Simplify to:

assert.NoError(t, err, "Should expand includes without error")

The test runner already provides full context via the subtest hierarchy.

includeFiles: map[string]string{
"include-no-engine.md": `---
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] This assertion uses require.Len, but the actual validation (engines content) comes after. If the length is wrong, the subsequent assertions will panic with index out of bounds. This is the correct use of require — good catch!

However, consider adding a more descriptive message:

require.Len(t, engines, tt.expectedLen, 
    "Engine count mismatch - check include file parsing")

This helps differentiate length failures from content failures when debugging.

tools:
github:
allowed: ["list_issues"]
---

# Include without Engine
`
includeFile := filepath.Join(tmpDir, "include-no-engine.md")
if err := os.WriteFile(includeFile, []byte(includeContent), 0644); err != nil {
t.Fatal(err)
}

// Create main markdown content with include directive
mainContent := `# Main Workflow
`,
},
mainContent: `# Main Workflow

@include include-no-engine.md

Some content here.
`

// Test engine expansion
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion, got error: %v", err)
}

// Should find no engines
if len(engines) != 0 {
t.Errorf("Expected 0 engines, got %d: %v", len(engines), engines)
}
}

func TestExpandIncludesForEnginesMultipleIncludes(t *testing.T) {
// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")

// Create first include file with engine
include1Content := `---
`,
expectedLen: 0,
},
{
name: "multiple includes",
includeFiles: map[string]string{
"include1.md": `---
engine: claude
tools:
github:
allowed: ["list_issues"]
---

# First Include
`
include1File := filepath.Join(tmpDir, "include1.md")
if err := os.WriteFile(include1File, []byte(include1Content), 0644); err != nil {
t.Fatal(err)
}

// Create second include file with engine
include2Content := `---
`,
"include2.md": `---
engine: codex
tools:
claude:
allowed: ["Read", "Write"]
---

# Second Include
`
include2File := filepath.Join(tmpDir, "include2.md")
if err := os.WriteFile(include2File, []byte(include2Content), 0644); err != nil {
t.Fatal(err)
}

// Create main markdown content with multiple include directives
mainContent := `# Main Workflow
`,
},
mainContent: `# Main Workflow

@include include1.md

Expand All @@ -186,49 +121,73 @@ Some content here.
@include include2.md

More content.
`
`,
expectedLen: 2,
expectedExact: map[int]string{0: `"claude"`, 1: `"codex"`},
},
{
name: "optional include missing file",
mainContent: `# Main Workflow

// Test engine expansion
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion, got error: %v", err)
}
@include? missing-file.md

// Should find two engines
if len(engines) != 2 {
t.Fatalf("Expected 2 engines, got %d: %v", len(engines), engines)
}
Some content here.
`,
expectedLen: 0,
},
{
name: "object engine with command",
includeFiles: map[string]string{
"include-command.md": `---
engine:
id: copilot
command: /custom/path/to/copilot
version: "1.0.0"
tools:
github:
allowed: ["list_issues"]
---

// Should extract both engines
if engines[0] != `"claude"` {
t.Errorf("Expected first engine 'claude', got %s", engines[0])
}
if engines[1] != `"codex"` {
t.Errorf("Expected second engine 'codex', got %s", engines[1])
}
}
# Include with Custom Command
`,
},
mainContent: `# Main Workflow

func TestExpandIncludesForEnginesOptionalMissing(t *testing.T) {
// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")
@include include-command.md

// Create main markdown content with optional include directive to non-existent file
mainContent := `# Main Workflow
Some content here.
`,
expectedLen: 1,
expectedContains: map[int][]string{
0: {`"id":"copilot"`, `"command":"/custom/path/to/copilot"`, `"version":"1.0.0"`},
},
},
}

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

Some content here.
`
engines, err := ExpandIncludesForEngines(tt.mainContent, tmpDir)
require.NoError(t, err, "Should expand includes for test case %q", tt.name)

// Test engine expansion - should not fail for optional includes
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion with optional missing include, got error: %v", err)
}
require.Len(t, engines, tt.expectedLen, "Should return expected number of engines for test case %q", tt.name)

for idx, expected := range tt.expectedExact {
assert.Equal(t, expected, engines[idx], "Engine %d should match expected value for test case %q", idx, tt.name)
}

// Should find no engines
if len(engines) != 0 {
t.Errorf("Expected 0 engines, got %d: %v", len(engines), engines)
for idx, expectedFields := range tt.expectedContains {
for _, expectedField := range expectedFields {
assert.Containsf(t, engines[idx], expectedField, "Engine %d should include field %q for test case %q", idx, expectedField, tt.name)
}
}
})
}
}

Expand Down Expand Up @@ -281,62 +240,8 @@ Just markdown content.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
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)
}
require.NoError(t, err, "Should extract engine field without error for case %q", tt.name)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Excellent refactor of TestExtractFrontmatterFieldEngine to use assert consistently and remove the duplication. This is exactly the right direction.

One minor improvement: the error message uses %q for tt.name redundantly (same issue as the other test). Simplify to:

require.NoError(t, err, "Should extract engine field without error")
assert.Equal(t, tt.expected, result, "Extracted engine should match expected value")

The subtest name already provides case context.

assert.Equal(t, tt.expected, result, "Extracted engine should match expected value for case %q", tt.name)
})
}
}

func TestExpandIncludesForEnginesWithCommand(t *testing.T) {
// Create temporary directory for test files
tmpDir := testutil.TempDir(t, "test-*")

// Create include file with engine command specification
includeContent := `---
engine:
id: copilot
command: /custom/path/to/copilot
version: "1.0.0"
tools:
github:
allowed: ["list_issues"]
---

# Include with Custom Command
`
includeFile := filepath.Join(tmpDir, "include-command.md")
if err := os.WriteFile(includeFile, []byte(includeContent), 0644); err != nil {
t.Fatal(err)
}

// Create main markdown content with include directive
mainContent := `# Main Workflow

@include include-command.md

Some content here.
`

// Test engine expansion
engines, err := ExpandIncludesForEngines(mainContent, tmpDir)
if err != nil {
t.Fatalf("Expected successful engine expansion, got error: %v", err)
}

// Should find one engine
if len(engines) != 1 {
t.Fatalf("Expected 1 engine, got %d", len(engines))
}

// Should extract engine object as JSON with command field
expectedFields := []string{`"id":"copilot"`, `"command":"/custom/path/to/copilot"`, `"version":"1.0.0"`}
for _, field := range expectedFields {
if !contains(engines[0], field) {
t.Errorf("Expected engine JSON to contain %s, got %s", field, engines[0])
}
}
}
Loading