[testify-expert] Test Quality Excellence: codemod_timeout_minutes_test.go - Best Practices Reference

[github-actions]

Overview

The test file pkg/cli/codemod_timeout_minutes_test.go has been selected for quality improvement by the Testify Uber Super Expert. This test file demonstrates excellent testify usage and test organization, serving as a model for other test files in the codebase. This issue documents best practices observed and provides minor enhancement opportunities.

Current State

• Test File: pkg/cli/codemod_timeout_minutes_test.go
• Source File: pkg/cli/codemod_timeout_minutes.go
• Test Functions: 8 test functions
• Lines of Code: 238 lines
• Primary Function Under Test: getTimeoutMinutesCodemod()

Test Quality Analysis

Strengths ✅

This test file exemplifies exceptional test quality and should serve as a reference for other test files:

1. Perfect Testify Usage - Every assertion uses appropriate testify functions (require.* for setup, assert.* for validations) with helpful messages
2. Comprehensive Test Coverage - Tests cover: basic migration, indentation preservation, comment preservation, edge cases (no field present), markdown body preservation, different values, and exact match behavior
3. Table-Driven Tests - Already uses table-driven tests where appropriate (see TestTimeoutMinutesCodemod_DifferentValues)
4. Clear Organization - Tests follow a logical progression from basic to complex scenarios
5. Excellent Naming - Test names clearly describe what being tested (e.g., TestTimeoutMinutesCodemod_PreservesComments)
6. Proper Build Tags - Correctly uses //go:build !integration tag

🎯 Minor Enhancement Opportunities

Minor Enhancement Opportunities 🎯

While this test file is already excellent, here are some minor enhancements to make it even better:

1. Test Helper Functions

Current State:
Each test manually creates the codemod, content, and frontmatter with some repetition.

Enhancement Opportunity:
Extract common test setup into helper functions to reduce duplication:

// Helper function to create test content
func createTestContent(timeoutValue int, extraFields ...string) string {
    base := fmt.Sprintf(`---
on: workflow_dispatch
timeout_minutes: %d`, timeoutValue)
    
    for _, field := range extraFields {
        base += "\n" + field
    }
    
    base += "\n---\n\n# Test Workflow"
    return base
}

// Helper function to create test frontmatter
func createTestFrontmatter(timeoutValue int, extra map[string]any) map[string]any {
    fm := map[string]any{
        "on":              "workflow_dispatch",
        "timeout_minutes": timeoutValue,
    }
    for k, v := range extra {
        fm[k] = v
    }
    return fm
}

// Helper function to run basic codemod assertions
func assertCodemodMigration(t *testing.T, result string, applied bool, err error, expectedValue int) {
    require.NoError(t, err, "Apply should not return an error")
    assert.True(t, applied, "Codemod should report changes")
    assert.Contains(t, result, fmt.Sprintf("timeout-minutes: %d", expectedValue), 
        "Result should contain new field with correct value")
    assert.NotContains(t, result, "timeout_minutes:", 
        "Result should not contain old field name")
}

Usage Example:

func TestTimeoutMinutesCodemod_BasicMigration(t *testing.T) {
    codemod := getTimeoutMinutesCodemod()
    
    content := createTestContent(30, "permissions:", "  contents: read")
    frontmatter := createTestFrontmatter(30, map[string]any{
        "permissions": map[string]any{"contents": "read"},
    })
    
    result, applied, err := codemod.Apply(content, frontmatter)
    assertCodemodMigration(t, result, applied, err, 30)
}

Why this matters: Helper functions reduce duplication, make tests more maintainable, and make the test intent clearer by hiding boilerplate.

2. Negative Test Cases - Error Conditions

Current State:
All tests assume successful execution. No tests verify error handling.

Enhancement Opportunity:
Add tests for potential error conditions:

func TestTimeoutMinutesCodemod_ErrorHandling(t *testing.T) {
    tests := []struct {
        name        string
        content     string
        frontmatter map[string]any
        expectError bool
        errorMsg    string
    }{
        {
            name: "malformed frontmatter",
            content: `---
on: workflow_dispatch
timeout_minutes: 30
---incomplete`,
            frontmatter: map[string]any{"timeout_minutes": 30},
            expectError: true,
            errorMsg:    "frontmatter",
        },
        {
            name: "empty content",
            content: "",
            frontmatter: map[string]any{"timeout_minutes": 30},
            expectError: true,
            errorMsg:    "empty",
        },
    }
    
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            codemod := getTimeoutMinutesCodemod()
            _, _, err := codemod.Apply(tt.content, tt.frontmatter)
            
            if tt.expectError {
                require.Error(t, err, "Should return error for %s", tt.name)
                assert.Contains(t, err.Error(), tt.errorMsg, 
                    "Error message should mention the issue")
            } else {
                require.NoError(t, err, "Should not error for %s", tt.name)
            }
        })
    }
}

Why this matters: Even if the current implementation doesn not have error paths, testing error conditions documents expected behavior and catches regressions if error handling is added later.

3. Integration with Helper Functions

Current State:
The test file doesn not test the integration with applyFrontmatterLineTransform and findAndReplaceInLine helper functions.

Enhancement Opportunity:
Consider adding a test that verifies the codemod interacts correctly with these helpers, especially for edge cases:

func TestTimeoutMinutesCodemod_MultipleOccurrences(t *testing.T) {
    codemod := getTimeoutMinutesCodemod()
    
    // Edge case: timeout_minutes appears multiple times in frontmatter
    content := `---
on: workflow_dispatch
timeout_minutes: 30
# Note: timeout_minutes should be migrated
---

# Test Workflow

The old timeout_minutes field is deprecated.`
    
    frontmatter := map[string]any{
        "on":              "workflow_dispatch",
        "timeout_minutes": 30,
    }
    
    result, applied, err := codemod.Apply(content, frontmatter)
    
    require.NoError(t, err, "Apply should not return an error")
    assert.True(t, applied, "Codemod should report changes")
    
    // Verify only frontmatter field is replaced, not comments or markdown
    assert.Contains(t, result, "timeout-minutes: 30", 
        "Frontmatter field should be replaced")
    assert.Contains(t, result, "# Note: timeout_minutes should be migrated", 
        "Comments in frontmatter should preserve original text")
    assert.Contains(t, result, "The old timeout_minutes field is deprecated.", 
        "Markdown body should preserve original text")
}

Why this matters: This tests the actual behavior of the codemod in realistic scenarios where the field name might appear in multiple contexts.

4. Test Coverage Documentation

Enhancement Opportunity:
Add a comment at the top of the test file documenting the test coverage strategy:

(go/redacted):build !integration

package cli

// Test Coverage for codemod_timeout_minutes.go
//
// This file tests the timeout_minutes -> timeout-minutes migration codemod.
// Coverage includes:
//   - Basic migration (field replacement)
//   - Indentation preservation
//   - Comment preservation (inline comments)
//   - No-op when field does not exist
//   - Markdown body preservation
//   - Multiple timeout values (table-driven)
//   - Exact field matching (does not affect similar field names)
//
// Future enhancements:
//   - Error handling tests
//   - Edge cases (malformed frontmatter, multiple occurrences)
//   - Integration tests with helper functions

import (
    // ... imports
)

Why this matters: Documentation helps future maintainers understand what tested and what gaps might exist.

📋 Implementation Guidelines

Implementation Guidelines

Priority Order

1. Optional: Add test helper functions (reduces duplication)
2. Optional: Add error handling tests (documents expected behavior)
3. Optional: Add edge case tests (multiple occurrences, field in comments)
4. Optional: Add test coverage documentation (helps future maintainers)

Note: These are all optional enhancements since the current test quality is already excellent.

Best Practices Demonstrated by This File ✅

This file already follows all patterns from scratchpad/testing.md:

• ✅ Uses require.* for critical setup (stops test on failure)
• ✅ Uses assert.* for test validations (continues checking)
• ✅ Includes table-driven tests with t.Run() and descriptive names
• ✅ Tests real component interactions (no mocks)
• ✅ All assertions include helpful messages
• ✅ Clear test organization and naming
• ✅ Comprehensive coverage of success paths and edge cases

Testing Commands

# Run tests for this file only
go test -v ./pkg/cli/ -run TestTimeoutMinutesCodemod

# Run with coverage
go test -cover ./pkg/cli/ -run TestTimeoutMinutesCodemod

# Run all CLI tests
go test -v ./pkg/cli/

# Run all tests
make test-unit

Acceptance Criteria

This file already meets all standard acceptance criteria. Optional enhancements:

• Extract common test setup into helper functions (optional)
• Add error handling test cases (optional)
• Add edge case tests for multiple occurrences (optional)
• Add test coverage documentation comment (optional)
• Tests continue to pass: make test-unit

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example Tests: This file itself is an excellent example to follow
• Testify Documentation: https://github.com/stretchr/testify

This test file demonstrates best-in-class test quality and should be referenced when writing new tests. The suggested enhancements are purely optional refinements.

---

Priority: Low (already excellent quality)
Effort: Small (optional enhancements only)
Expected Impact: Demonstrates best practices for other test files, minor maintainability improvements

Generated by 🧪 Daily Testify Uber Super Expert · ● 5.2M · ◷

• expires on May 21, 2026, 6:35 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 · ● 80.3K · ◷