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

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 using testify best practices.

### Current State

- **Test File**: `pkg/parser/frontmatter_utils_test.go`
- **Source Files**: `pkg/parser/remote_fetch.go`, `pkg/parser/import_processor.go`
- **Test Functions**: 5 test functions
- **Lines of Code**: 382 lines
- **Build Tag**: `//go:build !integration`

### Test Quality Analysis

### Strengths ✅

1. **Good table-driven test structure**: Most tests already use the `tests := []struct{...}` pattern with `t.Run()` subtests — this is the right approach.
2. **Good edge case coverage for `isUnderWorkflowsDirectory` and `isCustomAgentFile`**: Multiple meaningful boundary cases covered.
3. **Sensible test setup**: Uses `os.MkdirTemp` and proper cleanup with `defer os.RemoveAll`.

<details>
<summary><b>🎯 Areas for Improvement</b></summary>

#### 1. Replace Manual Error Checks with Testify Assertions

The entire file uses raw `t.Errorf` / `t.Fatalf` instead of testify assertions, which is inconsistent with the rest of the codebase (see `scratchpad/testing.md`).

**Current pattern (throughout the file):**
```go
// ❌ CURRENT — anti-patterns
if err != nil {
    t.Fatalf("Failed to create temp dir: %v", err)
}
if result != tt.expected {
    t.Errorf("isUnderWorkflowsDirectory(%q) = %v, want %v", tt.filePath, result, tt.expected)
}
if err == nil {
    t.Errorf("ResolveIncludePath() expected error, got nil")
}
```

**Recommended replacement:**
```go
// ✅ IMPROVED — testify assertions
tempDir, err := os.MkdirTemp("", "test_resolve")
require.NoError(t, err, "should create temp dir")
defer os.RemoveAll(tempDir)

// In subtests:
result := isUnderWorkflowsDirectory(tt.filePath)
assert.Equal(t, tt.expected, result, "isUnderWorkflowsDirectory(%q) should return expected result", tt.filePath)

// For error cases:
require.Error(t, err, "ResolveIncludePath() should return error for missing file")
```

**Why this matters**: Testify provides richer failure output, and `require.*` properly stops the test when critical setup fails rather than risking a nil-pointer panic in subsequent lines.

#### 2. Setup Code Should Use `require.*`

`TestResolveIncludePath` and `TestProcessImportsFromFrontmatter` use `t.Fatalf` for critical setup that must succeed:

```go
// ❌ CURRENT
if err := os.WriteFile(regularFile, []byte("test"), 0644); err != nil {
    t.Fatalf("Failed to write regular file: %v", err)
}

// ✅ IMPROVED
err = os.WriteFile(regularFile, []byte("test"), 0644)
require.NoError(t, err, "should write test fixture file")
```

Also, `TestProcessImportsFromFrontmatter` still uses `t.Fatalf` instead of `require.NoError` for file creation.

#### 3. Missing Tests for Unit-Testable Functions

The source file `remote_fetch.go` has two simple, pure functions with no external dependencies that are entirely untested:

**`isRepositoryImport(importPath string) bool`** — Determines if an import path is a remote repository reference (e.g. `owner/repo/path.md`). This function has non-trivial logic (strips `#section`, strips `@ref`, checks that path has 3+ slash-separated components) and deserves dedicated tests.

**`isNotFoundError(errMsg string) bool`** — Returns true when an error message contains "404" or "not found" (case-insensitive). Trivial to test.

**Recommended new tests:**
```go
func TestIsRepositoryImport(t *testing.T) {
    tests := []struct {
        name     string
        path     string
        expected bool
    }{
        {"owner/repo/file.md", "owner/repo/file.md", true},
        {"with ref", "owner/repo/file.md@main", true},
        {"with section", "owner/repo/file.md#section", true},
        {"with ref and section", "owner/repo/file.md@sha123#section", true},
        {"subdirectory", "owner/repo/sub/file.md", true},
        {"too few parts", "owner/repo", false},
        {"local relative", "./file.md", false},
        {"local .github", ".github/workflows/file.md", false},
        {"shared 2-part", "shared/file.md", false},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := isRepositoryImport(tt.path)
            assert.Equal(t, tt.expected, got, "isRepositoryImport(%q) should match expected", tt.path)
        })
    }
}

func TestIsNotFoundError(t *testing.T) {
    tests := []struct {
        name     string
        errMsg   string
        expected bool
    }{
        {"contains 404", "HTTP 404 response", true},
        {"contains not found", "resource not found", true},
        {"case insensitive", "NOT FOUND", true},
        {"mixed case 404", "Error: 404 Not Found", true},
        {"unrelated error", "connection refused", false},
        {"empty string", "", false},
        {"permission error", "403 Forbidden", false},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := isNotFoundError(tt.errMsg)
            assert.Equal(t, tt.expected, got, "isNotFoundError(%q) should return %v", tt.errMsg, tt.expected)
        })
    }
}
```

#### 4. `TestResolveIncludePath` Has Very Thin Coverage

Only 2 test cases exist ("regular relative path" and "regular file not found"). The function also handles:
- Absolute paths
- Paths that are workflow specs (remote imports via `isWorkflowSpec`)
- Symlink resolution

Recommended additional cases:
```go
{
    name:     "absolute path that exists",
    filePath: regularFile, // absolute path
    baseDir:  tempDir,
    expected: regularFile,
},
```

#### 5. Assertion Messages Are Missing

Most assertions lack descriptive messages, making failures harder to debug:

```go
// ❌ CURRENT — no message
assert.Equal(t, expected, result)

// ✅ IMPROVED — descriptive message
assert.Equal(t, expected, result, "isWorkflowSpec(%q) should return %v", tt.path, tt.want)
```

All `assert.*` and `require.*` calls should include a short message that explains what is being validated.

#### 6. Incomplete Test at End of File

The file ends with a comment-only test stub that was never implemented:
```go
// TestProcessIncludedFileWithNameAndDescription verifies that name and description fields
// do not generate warnings when processing included files outside .github/workflows/
```

This should either be implemented as a real test, or removed to avoid confusion.

</details>

<details>
<summary><b>📋 Implementation Guidelines</b></summary>

#### Priority Order
1. **High**: Add missing `TestIsRepositoryImport` and `TestIsNotFoundError` tests
2. **High**: Replace all `t.Errorf`/`t.Fatalf` with `assert.*`/`require.*` from testify
3. **Medium**: Add assertion messages to all assert/require calls
4. **Medium**: Expand `TestResolveIncludePath` with absolute path and additional edge cases
5. **Low**: Implement or remove the incomplete `TestProcessIncludedFileWithNameAndDescription` comment

#### Required Import
```go
import (
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)
```

#### Testing Commands
```bash
# Run just this package's tests
go test -v -run "TestIsUnderWorkflowsDirectory|TestIsCustomAgentFile|TestResolveIncludePath|TestIsWorkflowSpec|TestProcessImportsFromFrontmatter|TestIsRepositoryImport|TestIsNotFoundError" ./pkg/parser/

# Run full unit tests before committing
make test-unit
```

#### Best Practices Reference (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 — test real component interactions
- ✅ Always include helpful assertion messages

</details>

### Acceptance Criteria

- [ ] All `t.Errorf` / `t.Fatalf` replaced with `assert.*` / `require.*` testify assertions
- [ ] New `TestIsRepositoryImport` test added with ≥8 cases
- [ ] New `TestIsNotFoundError` test added with ≥6 cases
- [ ] `TestResolveIncludePath` expanded with absolute path case
- [ ] All assertions include descriptive messages
- [ ] Incomplete `TestProcessIncludedFileWithNameAndDescription` stub addressed
- [ ] Tests pass: `go test ./pkg/parser/` and `make test-unit`

### Additional Context

- **Repository Testing Guidelines**: See `scratchpad/testing.md` for comprehensive testing patterns
- **Example Tests**: `pkg/parser/schedule_parser_test.go` uses testify assertions well
- **Testify Documentation**: https://github.com/stretchr/testify
- **Workflow Run**: [§23846600281](https://github.com/github/gh-aw/actions/runs/23846600281)

---

**Priority**: Medium  
**Effort**: Small  
**Expected Impact**: Improved test quality, better failure messages, two additional unit-testable functions covered

**Files Involved:**
- Test file: `pkg/parser/frontmatter_utils_test.go`
- Source files: `pkg/parser/remote_fetch.go`, `pkg/parser/import_processor.go`







> Generated by [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/actions/runs/23846600281/agentic_workflow) · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw+is%3Aissue+%22gh-aw-workflow-call-id%3A+github%2Fgh-aw%2Fdaily-testify-uber-super-expert%22&type=issues)
> - [x] expires  on Apr 3, 2026, 11:46 AM UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Current State

Test Quality Analysis

Strengths ✅

1. Replace Manual Error Checks with Testify Assertions

2. Setup Code Should Use `require.*`

3. Missing Tests for Unit-Testable Functions

4. `TestResolveIncludePath` Has Very Thin Coverage

5. Assertion Messages Are Missing

6. Incomplete Test at End of File

Priority Order

Required Import

Testing Commands

Best Practices Reference (from `scratchpad/testing.md`)

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

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

Description

Current State

Test Quality Analysis

Strengths ✅

1. Replace Manual Error Checks with Testify Assertions

2. Setup Code Should Use require.*

3. Missing Tests for Unit-Testable Functions

4. TestResolveIncludePath Has Very Thin Coverage

5. Assertion Messages Are Missing

6. Incomplete Test at End of File

Priority Order

Required Import

Testing Commands

Best Practices Reference (from scratchpad/testing.md)

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

2. Setup Code Should Use `require.*`

4. `TestResolveIncludePath` Has Very Thin Coverage

Best Practices Reference (from `scratchpad/testing.md`)