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

[github-actions]

Overview

The test file pkg/workflow/resolve_test.go has been selected for quality improvement by the Testify Uber Super Expert. This file contains 10 test functions across 706 lines and tests the workflow resolution/lookup functionality (resolve.go). While it has good table-driven tests and broad coverage, it doesn't use testify at all — relying entirely on manual if/t.Fatal/t.Errorf patterns — and contains duplicated setup code and a custom substring helper that reinvents strings.Contains.

Current State

• Test File: pkg/workflow/resolve_test.go
• Source File: pkg/workflow/resolve.go
• Test Functions: 10 test functions
• Lines of Code: 706 lines
• Testify Usage: ❌ None — zero imports of testify/assert or testify/require

Test Quality Analysis

Strengths ✅

• Good table-driven patterns in TestNormalizeWorkflowName, TestResolveWorkflowName, TestFindWorkflowName, TestGetWorkflowLockFileName, and TestShouldSkipFirewallWorkflow
• Uses t.Chdir for clean directory management in most tests
• Covers core happy paths, error paths, and edge cases for all exported functions

🎯 Areas for Improvement

1. No Testify Assertions — The Biggest Gap

The file has zero testify usage. Every other test file in pkg/workflow/ uses testify (e.g., compiler_test.go imports assert and require). All assertions in resolve_test.go use raw t.Fatal/t.Errorf/t.Error calls.

Current (anti-pattern):

// Setup error – should stop the test
err := os.MkdirAll(workflowsDir, 0755)
if err != nil {
    t.Fatal(err)
}

// Validation – doesn't stop on failure
result, err := ResolveWorkflowName(tt.workflowInput)
if tt.expectError {
    if err == nil {
        t.Errorf("Expected error for workflow input %q, but got none", tt.workflowInput)
    }
} else {
    if err != nil {
        t.Errorf("Unexpected error for workflow input %q: %v", tt.workflowInput, err)
    }
    if result != tt.expectedWorkflowName {
        t.Errorf("Expected workflow name %q, got %q", tt.expectedWorkflowName, result)
    }
}

Improved (testify):

// Setup error – require stops the test immediately
require.NoError(t, os.MkdirAll(workflowsDir, 0755), "should create workflows directory")

// Validation – assert continues checking
result, err := ResolveWorkflowName(tt.workflowInput)
if tt.expectError {
    require.Error(t, err, "should return error for %q", tt.workflowInput)
} else {
    require.NoError(t, err, "should not return error for %q", tt.workflowInput)
    assert.Equal(t, tt.expectedWorkflowName, result, "workflow name should match")
}

This affects every test function in the file (all 10).

2. Custom contains / findSubstring Helpers That Reinvent strings.Contains

Lines 422–437 define two helper functions:

// contains checks if a string contains a substring
func contains(s, substr string) bool {
    return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
        (len(s) > len(substr) && s[len(s)-len(substr):] == substr) ||
        (len(s) > len(substr) && s[:len(substr)] == substr) ||
        (len(s) > len(substr) && findSubstring(s, substr)))
}

func findSubstring(s, substr string) bool {
    for i := 0; i <= len(s)-len(substr); i++ {
        if s[i:i+len(substr)] == substr {
            return true
        }
    }
    return false
}

This is a verbose reimplementation of strings.Contains. The contains function also has a logical flaw: if the substring appears only in the middle (not at start or end), the first three branches fail and it falls through to findSubstring, which saves it — but the overall complexity is unnecessary.

Improved:

// ❌ Remove both helper functions entirely.

// Replace usages like:
if err != nil && !contains(err.Error(), "Run 'gh aw compile'") {
    t.Errorf("Expected error to mention compilation, got: %v", err)
}

// With testify assert.Contains:
require.Error(t, err, "should return error for missing lock file")
assert.Contains(t, err.Error(), "gh aw compile", "error should suggest running compile")

3. Duplicated Setup Code Across Error-Path Tests

TestResolveWorkflowName_MissingLockFile, TestResolveWorkflowName_InvalidYAML, and TestResolveWorkflowName_MissingNameField all share identical setup boilerplate:

tempDir := testutil.TempDir(t, "test-*")
workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir())
err := os.MkdirAll(workflowsDir, 0755)
if err != nil {
    t.Fatal(err)
}
// ...create files...
t.Chdir(tempDir)

Improved — extract a helper:

// setupWorkflowDir creates a temp dir with the workflows subdirectory and changes into it.
// Returns the workflows directory path.
func setupWorkflowDir(t *testing.T) string {
    t.Helper()
    tempDir := testutil.TempDir(t, "test-*")
    workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir())
    require.NoError(t, os.MkdirAll(workflowsDir, 0755), "should create workflows directory")
    t.Chdir(tempDir)
    return workflowsDir
}

This reduces ~8 lines of boilerplate per test to a single call, improving readability and consistency.

4. TestResolveWorkflowName_ExistingAgenticWorkflow Uses Manual os.Chdir + Defer

This test manually calls os.Chdir and defers a restore:

err = os.Chdir(projectRoot)
if err != nil {
    t.Skipf("Cannot change to project root: %v", err)
}
defer func() {
    if err := os.Chdir(currentDir); err != nil {
        t.Errorf("Failed to restore working directory: %v", err)
    }
}()

All other tests in this file use t.Chdir(...) (Go 1.24+), which automatically restores the working directory after the test. This is both cleaner and safer.

Improved:

t.Chdir(projectRoot)  // t.Chdir auto-restores; no defer needed

5. Missing Assertion Messages Throughout

Most assertions lack descriptive messages, making failures hard to diagnose:

// ❌ No message — what went wrong?
if result != tt.expectedWorkflowName {
    t.Errorf("Expected workflow name %q, got %q", tt.expectedWorkflowName, result)
}

// ✅ With testify and message
assert.Equal(t, tt.expectedWorkflowName, result,
    "TestResolveWorkflowName[%s]: workflow name should match", tt.name)

6. TestGetAllWorkflows Uses Manual Map for Verification

workflowMap := make(map[string]string)
for _, wf := range workflows {
    workflowMap[wf.WorkflowID] = wf.DisplayName
}
for workflowID, expectedDisplayName := range testWorkflows {
    actualDisplayName, exists := workflowMap[workflowID]
    if !exists {
        t.Errorf("Expected workflow ID %q not found in results", workflowID)
    } else if actualDisplayName != expectedDisplayName { ...}
}

Improved with require.Len and assert.Contains:

require.Len(t, workflows, len(testWorkflows), "should return correct number of workflows")

// Build map once with require
workflowMap := make(map[string]string, len(workflows))
for _, wf := range workflows {
    workflowMap[wf.WorkflowID] = wf.DisplayName
}
for id, expectedName := range testWorkflows {
    assert.Equal(t, expectedName, workflowMap[id],
        "workflow %q should have correct display name", id)
}

📋 Implementation Guidelines

Priority Order

1. High: Add testify/assert and testify/require imports; replace all t.Fatal/t.Errorf/t.Error with testify equivalents
2. High: Remove custom contains/findSubstring helpers; use assert.Contains / strings.Contains
3. Medium: Extract setupWorkflowDir helper to eliminate duplicated setup boilerplate
4. Medium: Replace manual os.Chdir + defer in TestResolveWorkflowName_ExistingAgenticWorkflow with t.Chdir
5. Low: Add assertion messages throughout for better failure diagnostics

Required Import Change

import (
    "os"
    "path/filepath"
    "strings"
    "testing"

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

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 these specific tests
go test -v -run "TestNormalizeWorkflowName|TestResolveWorkflowName|TestFindWorkflowName|TestGetAllWorkflows|TestGetWorkflowLockFileName|TestShouldSkipFirewallWorkflow" ./pkg/workflow/

# Run all tests after changes
make test-unit

Acceptance Criteria

• testify/assert and testify/require imported and used throughout
• All t.Fatal(err) replaced with require.NoError(t, err, "...")
• All if err == nil { t.Errorf("Expected error...") } replaced with require.Error(t, err, "...")
• All if result != expected { t.Errorf(...) } replaced with assert.Equal(t, expected, result, "...")
• Custom contains and findSubstring helpers removed; replaced with strings.Contains / assert.Contains
• setupWorkflowDir helper extracted to eliminate duplicated boilerplate
• TestResolveWorkflowName_ExistingAgenticWorkflow uses t.Chdir instead of os.Chdir + defer
• All assertions include helpful messages
• Tests pass: go test -v ./pkg/workflow/ -run "TestNormalizeWorkflowName|TestResolveWorkflowName|TestFindWorkflowName|TestGetAllWorkflows|TestGetWorkflowLockFileName|TestShouldSkipFirewallWorkflow"
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example with Testify: pkg/workflow/compiler_test.go — uses assert and require throughout
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small — mechanical replacement of assertion style across ~706 lines
Expected Impact: Consistent assertion style, clearer failure messages, less duplicated boilerplate, removal of buggy/unnecessary custom helpers

Files Involved:

• Test file: pkg/workflow/resolve_test.go
• Source file: pkg/workflow/resolve.go

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

• expires on May 26, 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 22.7K · ◷