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

[github-actions]

Overview

The test file pkg/parser/frontmatter_extraction_test.go has been selected for quality improvement by the Testify Uber Super Expert. This file tests the core frontmatter parsing logic in pkg/parser/frontmatter_content.go and is solid structurally (table-driven, subtests), but relies on raw t.Errorf/t.Fatalf throughout instead of testify assertions, and leaves several exported functions untested.

Current State

• Test File: pkg/parser/frontmatter_extraction_test.go
• Source File: pkg/parser/frontmatter_content.go
• Test Functions: 5 test functions
• Lines of Code: 366 lines
• Source Functions (exported): ExtractFrontmatterFromContent, ExtractFrontmatterFromBuiltinFile, ExtractMarkdownSection, ExtractMarkdownContent, ExtractWorkflowNameFromMarkdownBody, ExtractWorkflowNameFromContent

Test Quality Analysis

Strengths ✅

• All test functions use table-driven patterns with t.Run() subtests — excellent structure
• Good edge case coverage for ExtractFrontmatterFromContent (no-break whitespace, CRLF, whitespace-padded delimiters)
• Tests are in the same parser package (white-box), enabling testing of unexported generateDefaultWorkflowName

🎯 Areas for Improvement

1. Testify Assertions Missing Throughout

Every test in this file uses raw t.Errorf/t.Fatalf instead of testify. This makes failures harder to read and adds unnecessary boilerplate.

Current Pattern (anti-pattern):

if tt.wantErr {
    if err == nil {
        t.Errorf("ExtractFrontmatterFromContent() expected error, got nil")
    }
    return
}
if err != nil {
    t.Errorf("ExtractFrontmatterFromContent() error = %v", err)
    return
}
if result.Markdown != tt.wantMarkdown {
    t.Errorf("ExtractFrontmatterFromContent() markdown = %v, want %v", result.Markdown, tt.wantMarkdown)
}

Recommended Pattern (testify):

if tt.wantErr {
    require.Error(t, err, "should return error for invalid input")
    return
}
require.NoError(t, err, "should parse valid frontmatter without error")
assert.Equal(t, tt.wantMarkdown, result.Markdown, "markdown body should match expected")

The require.NoError call stops the test immediately if the assertion fails, preventing confusing nil-pointer panics in subsequent assertions — exactly what the manual return was trying to achieve, but with better error output.

2. Manual Map Iteration Instead of assert.Equal

TestExtractFrontmatterFromContent manually iterates the expected map and checks each key individually. This misses cases where the result has extra keys not in tt.wantYAML.

Current (incomplete + verbose):

if len(tt.wantYAML) != len(result.Frontmatter) {
    t.Errorf("frontmatter length = %v, want %v", len(result.Frontmatter), len(tt.wantYAML))
}
for key, expectedValue := range tt.wantYAML {
    if actualValue, exists := result.Frontmatter[key]; !exists {
        t.Errorf("missing key %v", key)
    } else if actualValue != expectedValue {
        t.Errorf("frontmatter[%v] = %v, want %v", key, actualValue, expectedValue)
    }
}

Recommended:

assert.Equal(t, tt.wantYAML, result.Frontmatter, "frontmatter map should match expected keys and values")

assert.Equal uses reflect.DeepEqual internally and prints a clear diff on failure, making the length check and the loop completely redundant.

3. reflect.DeepEqual Used Directly

TestExtractFrontmatterFromContent_FrontmatterLinesAndStart imports reflect just to call reflect.DeepEqual for slice comparison:

if !reflect.DeepEqual(result.FrontmatterLines, tt.wantFrontmatterLines) {
    t.Errorf("FrontmatterLines = %#v, want %#v", result.FrontmatterLines, tt.wantFrontmatterLines)
}

Recommended:

assert.Equal(t, tt.wantFrontmatterLines, result.FrontmatterLines, "frontmatter lines should match")
assert.Equal(t, tt.wantFrontmatterStart, result.FrontmatterStart, "frontmatter start line should match")

This removes the reflect import entirely and produces better diff output on failure.

4. Missing Tests for Exported Functions

Three exported functions in frontmatter_content.go have no test coverage:

Function	Description
ExtractFrontmatterFromBuiltinFile	Parses frontmatter from embedded file bytes
ExtractWorkflowNameFromMarkdownBody	Extracts workflow name from H1 heading or path
ExtractWorkflowNameFromContent	Combines frontmatter parse + name extraction

Priority Functions to Add:

ExtractWorkflowNameFromContent — This is the top-level API and deserves comprehensive table-driven tests:

func TestExtractWorkflowNameFromContent(t *testing.T) {
    tests := []struct {
        name        string
        content     string
        virtualPath string
        expected    string
        shouldErr   bool
    }{
        {
            name:        "name from H1 heading",
            content:     "---\nengine: copilot\n---\n\n# My Workflow\n\nDescription.",
            virtualPath: "my-workflow.md",
            expected:    "My Workflow",
        },
        {
            name:        "name from file path when no H1",
            content:     "---\nengine: copilot\n---\n\nDescription without heading.",
            virtualPath: "deploy-to-production.md",
            expected:    "Deploy To Production",
        },
        {
            name:        "unclosed frontmatter returns error",
            content:     "---\nengine: copilot",
            virtualPath: "workflow.md",
            shouldErr:   true,
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := ExtractWorkflowNameFromContent(tt.content, tt.virtualPath)
            if tt.shouldErr {
                require.Error(t, err)
                return
            }
            require.NoError(t, err)
            assert.Equal(t, tt.expected, result)
        })
    }
}

ExtractWorkflowNameFromMarkdownBody — Should test the H1 detection and fallback path:

func TestExtractWorkflowNameFromMarkdownBody(t *testing.T) {
    tests := []struct {
        name        string
        markdownBody string
        virtualPath  string
        expected     string
    }{
        {"h1 heading used",         "# Deploy App\n\nContent.",   "ignored.md",        "Deploy App"},
        {"fallback to file path",   "No heading here.",            "build-pipeline.md", "Build Pipeline"},
        {"empty body falls back",   "",                            "run-tests.md",      "Run Tests"},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := ExtractWorkflowNameFromMarkdownBody(tt.markdownBody, tt.virtualPath)
            require.NoError(t, err)
            assert.Equal(t, tt.expected, result, "workflow name should be extracted correctly")
        })
    }
}

5. Missing Assertion Messages

Many assertions lack descriptive messages, making CI failure output difficult to interpret without re-reading the test source:

// ❌ CURRENT — no message
if result != tt.expected {
    t.Errorf("ExtractMarkdownSection() = %q, want %q", result, tt.expected)
}

// ✅ IMPROVED — testify with message
assert.Equal(t, tt.expected, result,
    "section %q should be extracted correctly from markdown", tt.sectionName)

📋 Implementation Guidelines

Priority Order

1. High: Replace t.Errorf/t.Fatalf with require.NoError/assert.Equal throughout
2. High: Replace manual map iteration with assert.Equal (removes reflect import too)
3. High: Add tests for ExtractWorkflowNameFromContent and ExtractWorkflowNameFromMarkdownBody
4. Medium: Add test for ExtractFrontmatterFromBuiltinFile
5. Low: Add descriptive messages to all assertions

Required Imports After Migration

import (
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

The reflect import can be removed entirely once assert.Equal replaces reflect.DeepEqual.

Best Practices from scratchpad/testing.md

• ✅ Use require.* for critical assertions that make subsequent steps meaningless on failure
• ✅ Use assert.* for independent validations that can accumulate failures
• ✅ Table-driven tests — already present, keep and extend
• ✅ Always include helpful assertion messages
• ✅ No mocks — already good, tests call real parser functions

Testing Commands

# Run tests for this file
go test -v ./pkg/parser/ -run "TestExtract|TestGenerate"

# Run with coverage
go test -cover ./pkg/parser/

# Full unit suite
make test-unit

Acceptance Criteria

• All t.Errorf/t.Fatalf replaced with testify assert.*/require.* assertions
• Manual map iteration in TestExtractFrontmatterFromContent replaced with assert.Equal
• reflect.DeepEqual replaced with assert.Equal (remove reflect import)
• Tests added for ExtractWorkflowNameFromContent
• Tests added for ExtractWorkflowNameFromMarkdownBody
• All assertions include descriptive messages
• Tests pass: go test -v ./pkg/parser/ -run "TestExtract|TestGenerate"
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md
• Source File: pkg/parser/frontmatter_content.go — 17 functions, 5 exported currently untested
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (mostly mechanical testify migration) + Medium (new test functions)
Expected Impact: Clearer test failure output, removed boilerplate, improved coverage of workflow name extraction logic

Files Involved:

• Test file: pkg/parser/frontmatter_extraction_test.go
• Source file: pkg/parser/frontmatter_content.go

References:

• §26720553623

Generated by 🧪 Daily Testify Uber Super Expert · sonnet46 2.6M · ◷

• expires on Jun 2, 2026, 6:23 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 · haiku45 20.1K · ◷