[testify-expert] Improve Test Quality: pkg/workflow/compiler_test.go

The test file `pkg/workflow/compiler_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.

### Overview

- **Test File**: `pkg/workflow/compiler_test.go`
- **Source File**: `pkg/workflow/compiler.go`
- **Test Functions**: 11 test functions
- **Lines of Code**: 299 lines
- **Source Functions**: 2 exported methods (`CompileWorkflow`, `CompileWorkflowData`)

### Test Quality Analysis

#### Strengths ✅

1. **Excellent testify usage** - The tests consistently use `require.NoError()` for setup and `assert.Contains()`, `assert.Equal()` for validations
2. **Good test organization** - Tests are well-named following the `Test(Function)_(Scenario)` pattern
3. **Comprehensive error path coverage** - Tests cover nonexistent files, empty paths, missing frontmatter, invalid frontmatter, etc.
4. **Proper use of `testutil.TempDir()`** - All tests that need temporary directories use the helper function correctly

### Areas for Improvement 🎯

#### 1. Convert Similar Tests to Table-Driven Pattern

**Current Issue:**
Multiple test functions test similar error scenarios with repeated patterns. Tests like `TestCompileWorkflow_NonexistentFile`, `TestCompileWorkflow_EmptyPath`, `TestCompileWorkflow_MissingFrontmatter`, etc., follow nearly identical structures.

**Recommended Changes:**

``````go
// ✅ IMPROVED - Consolidated error scenarios in table-driven test
func TestCompileWorkflow_ErrorScenarios(t *testing.T) {
    tests := []struct {
        name          string
        setupFunc     func(t *testing.T) string  // Returns file path to test
        expectedError string
    }{
        {
            name: "nonexistent file",
            setupFunc: func(t *testing.T) string {
                return "/nonexistent/file.md"
            },
            expectedError: "failed to read file",
        },
        {
            name: "empty path",
            setupFunc: func(t *testing.T) string {
                return ""
            },
            expectedError: "", // Just check error exists
        },
        {
            name: "missing frontmatter",
            setupFunc: func(t *testing.T) string {
                tmpDir := testutil.TempDir(t, "compiler-missing-frontmatter")
                testFile := filepath.Join(tmpDir, "no-frontmatter.md")
                content := `# Test Workflow

This workflow has no frontmatter.
`
                require.NoError(t, os.WriteFile(testFile, []byte(content), 0644))
                return testFile
            },
            expectedError: "frontmatter",
        },
        {
            name: "invalid YAML frontmatter",
            setupFunc: func(t *testing.T) string {
                tmpDir := testutil.TempDir(t, "compiler-invalid-frontmatter")
                testFile := filepath.Join(tmpDir, "invalid-frontmatter.md")
                content := `---
on: push
invalid yaml: [unclosed bracket
---

# Test Workflow
`
                require.NoError(t, os.WriteFile(testFile, []byte(content), 0644))
                return testFile
            },
            expectedError: "", // Any error is acceptable
        },
        {
            name: "missing markdown content",
            setupFunc: func(t *testing.T) string {
                tmpDir := testutil.TempDir(t, "compiler-no-markdown")
                testFile := filepath.Join(tmpDir, "no-markdown.md")
                content := `---
on: push
engine: copilot
---
`
                require.NoError(t, os.WriteFile(testFile, []byte(content), 0644))
                return testFile
            },
            expectedError: "markdown content",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            filePath := tt.setupFunc(t)
            
            compiler := NewCompiler()
            err := compiler.CompileWorkflow(filePath)
            
            require.Error(t, err, "Should error for %s", tt.name)
            
            if tt.expectedError != "" {
                assert.Contains(t, err.Error(), tt.expectedError,
                    "Error should mention %s", tt.expectedError)
            }
        })
    }
}
``````

**Why this matters**: Consolidating similar error tests makes it easier to add new error scenarios and reduces code duplication. This follows the table-driven pattern used throughout the codebase (see `scratchpad/testing.md`).

#### 2. Add Test for Helper Functions

**Missing Tests:**

The source file contains helper functions that are not directly tested:
1. **`formatCompilerError`** (line 44 in compiler.go) - Creates formatted compiler error messages
2. **`formatCompilerMessage`** (line 61 in compiler.go) - Creates formatted compiler message strings

**Recommended Test Cases:**

``````go
func TestFormatCompilerError(t *testing.T) {
    tests := []struct {
        name     string
        filePath string
        errType  string
        message  string
    }{
        {
            name:     "basic error",
            filePath: "test.md",
            errType:  "error",
            message:  "test error message",
        },
        {
            name:     "warning type",
            filePath: "/path/to/workflow.md",
            errType:  "warning",
            message:  "test warning",
        },
        {
            name:     "long file path",
            filePath: "/very/long/path/to/some/workflow/file.md",
            errType:  "error",
            message:  "validation failed",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            err := formatCompilerError(tt.filePath, tt.errType, tt.message)
            
            require.Error(t, err, "Should return an error")
            
            errStr := err.Error()
            assert.Contains(t, errStr, tt.filePath, 
                "Error should contain file path")
            assert.Contains(t, errStr, tt.message,
                "Error should contain the message")
        })
    }
}

func TestFormatCompilerMessage(t *testing.T) {
    tests := []struct {
        name     string
        filePath string
        msgType  string
        message  string
    }{
        {
            name:     "error message",
            filePath: "test.md",
            msgType:  "error",
            message:  "test error",
        },
        {
            name:     "warning message",
            filePath: "workflow.md",
            msgType:  "warning",
            message:  "test warning",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result := formatCompilerMessage(tt.filePath, tt.msgType, tt.message)
            
            assert.NotEmpty(t, result, "Should return non-empty message")
            assert.Contains(t, result, tt.filePath,
                "Message should contain file path")
            assert.Contains(t, result, tt.message,
                "Message should contain the message text")
        })
    }
}
``````

**Why this matters**: Testing helper functions ensures they format messages correctly and helps catch regressions when error formatting logic changes.

#### 3. Enhance Edge Case Testing

**Current Gap:**
The path traversal test (`TestCompileWorkflow_PathTraversal`) only tests `../../etc/passwd` but could be more comprehensive.

**Recommended Improvements:**

``````go
func TestCompileWorkflow_PathTraversal(t *testing.T) {
    tests := []struct {
        name string
        path string
    }{
        {
            name: "parent directory traversal",
            path: "../../etc/passwd",
        },
        {
            name: "absolute path to system file",
            path: "/etc/passwd",
        },
        {
            name: "mixed traversal",
            path: "/tmp/../etc/passwd",
        },
        {
            name: "windows-style path",
            path: "..\\..\\windows\\system32",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            compiler := NewCompiler()
            err := compiler.CompileWorkflow(tt.path)
            
            require.Error(t, err, 
                "Should error for path traversal attempt: %s", tt.path)
        })
    }
}
``````

#### 4. Add Tests for Compiler State Management

**Missing Coverage:**
While `TestCompileWorkflowData_ArtifactManagerReset` tests artifact manager, there's no test for the `markdownPath` field management that's set in both `CompileWorkflow` and `CompileWorkflowData`.

**Recommended Test:**

``````go
func TestCompiler_MarkdownPathManagement(t *testing.T) {
    tmpDir := testutil.TempDir(t, "compiler-path-test")
    
    compiler := NewCompiler()
    
    // Create first workflow
    path1 := filepath.Join(tmpDir, "workflow1.md")
    content := `---
on: push
engine: copilot
---

# Workflow 1
`
    require.NoError(t, os.WriteFile(path1, []byte(content), 0644))
    
    // Compile first workflow
    err := compiler.CompileWorkflow(path1)
    require.NoError(t, err, "First compilation should succeed")
    assert.Equal(t, path1, compiler.markdownPath,
        "Compiler should store first markdown path")
    
    // Create and compile second workflow
    path2 := filepath.Join(tmpDir, "workflow2.md")
    require.NoError(t, os.WriteFile(path2, []byte(content), 0644))
    
    err = compiler.CompileWorkflow(path2)
    require.NoError(t, err, "Second compilation should succeed")
    assert.Equal(t, path2, compiler.markdownPath,
        "Compiler should update to second markdown path")
}
``````

#### 5. Improve Assertion Messages

**Current Issues:**
Some assertions lack descriptive messages that would aid in debugging test failures.

**Examples to Improve:**

``````go
// ❌ CURRENT (line 206-207)
assert.LessOrEqual(t, info.Size(), int64(MaxLockFileSize),
    "Lock file should not exceed MaxLockFileSize (%d bytes)", MaxLockFileSize)

// ✅ IMPROVED
assert.LessOrEqual(t, info.Size(), int64(MaxLockFileSize),
    "Lock file size %d bytes should not exceed MaxLockFileSize limit of %d bytes", 
    info.Size(), MaxLockFileSize)

// ❌ CURRENT (line 233-234)
assert.True(t, strings.Contains(errorStr, "invalid.md") || strings.Contains(errorStr, "error"),
    "Error should reference the file or contain 'error'")

// ✅ IMPROVED
assert.True(t, strings.Contains(errorStr, "invalid.md") || strings.Contains(errorStr, "error"),
    "Error message should reference file 'invalid.md' or contain 'error', got: %q", 
    errorStr)
``````

### Implementation Guidelines

#### Priority Order
1. **High**: Add tests for `formatCompilerError` and `formatCompilerMessage` helper functions
2. **High**: Refactor error scenario tests into table-driven pattern
3. **Medium**: Enhance path traversal tests with more edge cases
4. **Medium**: Add test for `markdownPath` state management
5. **Low**: Improve assertion messages throughout

#### 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
``````bash
# Run tests for this file
go test -v ./pkg/workflow/ -run TestCompile

# Run specific test
go test -v ./pkg/workflow/ -run TestCompileWorkflow_ValidWorkflow

# Run tests with coverage
go test -cover ./pkg/workflow/

# Run all unit tests
make test-unit
``````

### Acceptance Criteria

- [ ] Add table-driven test consolidating error scenarios (replaces 5 existing tests)
- [ ] Add tests for `formatCompilerError` and `formatCompilerMessage` functions
- [ ] Enhance path traversal test with more edge cases
- [ ] Add test for `markdownPath` state management
- [ ] Improve assertion messages to include actual values for easier debugging
- [ ] All tests pass: `make test-unit`
- [ ] Code follows patterns in `scratchpad/testing.md`

### Additional Context

- **Repository Testing Guidelines**: See `scratchpad/testing.md` for comprehensive testing patterns
- **Example Tests**: Look at recent test files in `pkg/workflow/*_test.go` for similar patterns
- **Testify Documentation**: https://github.com/stretchr/testify

---

**Priority**: Medium  
**Effort**: Medium (4-6 hours)  
**Expected Impact**: Improved test maintainability, better coverage of helper functions, easier debugging with improved assertions

**Files Involved:**
- Test file: `pkg/workflow/compiler_test.go`
- Source file: `pkg/workflow/compiler.go`







> AI generated by [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/actions/runs/21628229022)
> - [x] expires  on Feb 5, 2026, 11:27 AM UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[testify-expert] Improve Test Quality: pkg/workflow/compiler_test.go #13462

Overview

Test Quality Analysis

Strengths ✅

Areas for Improvement 🎯

1. Convert Similar Tests to Table-Driven Pattern

2. Add Test for Helper Functions

3. Enhance Edge Case Testing

4. Add Tests for Compiler State Management

5. Improve Assertion Messages

Implementation Guidelines

Priority Order

Best Practices from `scratchpad/testing.md`

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[testify-expert] Improve Test Quality: pkg/workflow/compiler_test.go #13462

Description

Overview

Test Quality Analysis

Strengths ✅

Areas for Improvement 🎯

1. Convert Similar Tests to Table-Driven Pattern

2. Add Test for Helper Functions

3. Enhance Edge Case Testing

4. Add Tests for Compiler State Management

5. Improve Assertion Messages

Implementation Guidelines

Priority Order

Best Practices from scratchpad/testing.md

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Best Practices from `scratchpad/testing.md`