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

[github-actions]

Overview

The test file pkg/workflow/compiler_reactions_numeric_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/workflow/compiler_reactions_numeric_test.go
• Test Functions: 3 test functions
• Lines of Code: 188 lines
• Testify Usage: ❌ None - uses manual t.Fatal() and t.Errorf() patterns

Test Functions:

• TestInvalidReactionValue - Tests invalid reaction validation
• TestNumericReactionParsing - Tests +1/-1 reaction parsing (table-driven ✅)
• TestInvalidNumericReaction - Tests invalid numeric reaction (value: 2)

Test Quality Analysis

Strengths ✅

1. Table-Driven Pattern: TestNumericReactionParsing uses proper table-driven test structure with t.Run() subtests
2. Good Test Coverage: Tests both valid parsing scenarios (quoted/unquoted +1/-1) and invalid cases
3. Clear Test Names: Descriptive test case names explain the scenario being tested

Areas for Improvement 🎯

1. Testify Assertions (HIGH PRIORITY)

Current Issues:

• ❌ No testify imports - File uses manual error checking with t.Fatal(), t.Fatalf(), and t.Errorf()
• ❌ Manual error handling - 11 occurrences of manual error checks instead of testify assertions
• ❌ Less informative failures - Manual assertions provide less context on failures

Recommended Changes:

// ❌ CURRENT (lines 42, 125, 170)
if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil {
    t.Fatal(err)
}

// ✅ IMPROVED (testify)
err := os.WriteFile(testFile, []byte(testContent), 0644)
require.NoError(t, err, "Failed to write test file")

// ❌ CURRENT (line 51)
var err error
_, err = compiler.ParseWorkflowFile(testFile)
if err == nil {
    t.Fatal("Expected error for invalid reaction value, but got none")
}

// ✅ IMPROVED (testify)
_, err := compiler.ParseWorkflowFile(testFile)
require.Error(t, err, "Expected error for invalid reaction value")

// ❌ CURRENT (lines 61, 64)
errMsg := err.Error()
hasInvalidValue := strings.Contains(errMsg, "invalid_emoji") || strings.Contains(errMsg, "reaction")
hasValidOptions := strings.Contains(errMsg, "must be one of") || strings.Contains(errMsg, "+1") || strings.Contains(errMsg, "eyes")

if !hasInvalidValue {
    t.Errorf("Error message should mention the invalid reaction value, got: %v", err)
}
if !hasValidOptions {
    t.Errorf("Error message should mention valid reaction options, got: %v", err)
}

// ✅ IMPROVED (testify with better assertions)
errMsg := err.Error()
assert.Contains(t, errMsg, "reaction", "Error should mention 'reaction' or 'invalid_emoji'")
assert.True(t, 
    strings.Contains(errMsg, "must be one of") || 
    strings.Contains(errMsg, "+1") || 
    strings.Contains(errMsg, "eyes"),
    "Error message should list valid reaction options, got: %v", err)

// ❌ CURRENT (line 136)
if workflowData.AIReaction != tc.expectedReaction {
    t.Errorf("Expected AIReaction to be %q, got %q", tc.expectedReaction, workflowData.AIReaction)
}

// ✅ IMPROVED (testify)
assert.Equal(t, tc.expectedReaction, workflowData.AIReaction, 
    "AIReaction should match expected value for %s", tc.reactionInYAML)

Required Import:

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

    "github.com/github/gh-aw/pkg/testutil"
    "github.com/stretchr/testify/assert"   // ADD
    "github.com/stretchr/testify/require"  // ADD
)

Why this matters: Testify provides:

• ✅ Clearer error messages with expected vs actual values
• ✅ Better test output formatting
• ✅ Consistent assertion style across the codebase (see scratchpad/testing.md)
• ✅ Distinction between setup failures (require) and validation failures (assert)

2. Refine Error Message Validation (MEDIUM PRIORITY)

Current Issue:

• Lines 57-65: Complex boolean logic to check error message content
• Lines 183-185: Similar pattern with less comprehensive checking

Recommended Improvements:

// ✅ IMPROVED - More precise error message validation
func TestInvalidReactionValue(t *testing.T) {
    tmpDir := testutil.TempDir(t, "invalid-reaction-test")
    testContent := `...` // Same content
    
    testFile := filepath.Join(tmpDir, "test-invalid-reaction.md")
    err := os.WriteFile(testFile, []byte(testContent), 0644)
    require.NoError(t, err, "Failed to write test file")
    
    compiler := NewCompiler()
    _, err = compiler.ParseWorkflowFile(testFile)
    require.Error(t, err, "Expected error for invalid reaction value")
    
    // More precise error validation using testify
    errMsg := err.Error()
    assert.Contains(t, errMsg, "reaction", "Error should reference 'reaction' field")
    
    // Check for valid options (at least one should be mentioned)
    validOptions := []string{"must be one of", "+1", "-1", "eyes", "heart", "rocket", "tada"}
    hasValidOption := false
    for _, option := range validOptions {
        if strings.Contains(errMsg, option) {
            hasValidOption = true
            break
        }
    }
    assert.True(t, hasValidOption, 
        "Error message should mention valid reaction options, got: %v", errMsg)
}

3. Add Edge Case Tests (MEDIUM PRIORITY)

Missing Test Scenarios:

The file tests basic validation but could benefit from additional edge cases:

1. Empty/nil reaction value - What happens with reaction: ""?
2. Case sensitivity - Does reaction: Eyes vs reaction: eyes work?
3. Whitespace handling - Does reaction: " eyes " get trimmed?
4. Reaction "none" - Based on compiler_safe_outputs.go:87, "none" is valid - is it tested?
5. Default reaction - Line 133 sets default "eyes" for commands - test coverage?

Recommended Test Addition:

func TestReactionEdgeCases(t *testing.T) {
    tests := []struct {
        name          string
        reactionValue string
        shouldError   bool
        expectedValue string
    }{
        {
            name:          "empty string should error",
            reactionValue: `""`,
            shouldError:   true,
        },
        {
            name:          "none disables reactions",
            reactionValue: `"none"`,
            shouldError:   false,
            expectedValue: "none",
        },
        {
            name:          "whitespace trimmed",
            reactionValue: `" eyes "`,
            shouldError:   false,
            expectedValue: "eyes",
        },
        {
            name:          "case sensitivity",
            reactionValue: `"Eyes"`,
            shouldError:   true, // Assuming lowercase required
        },
    }
    
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            tmpDir := testutil.TempDir(t, "reaction-edge-case-test")
            testContent := fmt.Sprintf(`---
on:
  issues:
    types: [opened]
  reaction: %s
permissions:
  contents: read
  issues: write
strict: false
tools:
  github:
    allowed: [issue_read]
---

# Reaction Edge Case Test
`, tt.reactionValue)
            
            testFile := filepath.Join(tmpDir, "test-edge-case.md")
            err := os.WriteFile(testFile, []byte(testContent), 0644)
            require.NoError(t, err, "Failed to write test file")
            
            compiler := NewCompiler()
            workflowData, err := compiler.ParseWorkflowFile(testFile)
            
            if tt.shouldError {
                require.Error(t, err, "Expected error for reaction value: %s", tt.reactionValue)
            } else {
                require.NoError(t, err, "Should parse successfully for reaction value: %s", tt.reactionValue)
                assert.Equal(t, tt.expectedValue, workflowData.AIReaction, 
                    "AIReaction should match expected value")
            }
        })
    }
}

4. Test Organization and Documentation (LOW PRIORITY)

Current Issues:

• Good: Each test function has clear purpose
• Could improve: Add package-level documentation explaining what's being tested

Recommended Improvement:

//go:build !integration

package workflow

// This file tests GitHub AI reaction parsing and validation.
// 
// GitHub reactions are emojis that can be added to issues/PRs when workflows
// trigger. The workflow compiler must:
// - Parse string reactions ("eyes", "heart", "+1", "-1", etc.)
// - Handle numeric reactions (YAML parses unquoted +1 as integer 1)
// - Validate reaction values against allowed list
// - Convert numeric values back to string format (1 → "+1", -1 → "-1")
//
// Related code:
// - pkg/workflow/compiler_safe_outputs.go - Reaction extraction logic
// - pkg/workflow/compiler_types.go:423 - AIReaction field definition

import (
    // ... imports
)

Implementation Guidelines

Priority Order

1. 🔴 HIGH: Convert all manual error checks to testify assertions
2. 🟡 MEDIUM: Refine error message validation logic
3. 🟡 MEDIUM: Add edge case tests for "none", empty, whitespace, case sensitivity
4. 🟢 LOW: Add package-level documentation

Step-by-Step Migration

1. Add testify imports (lines 5-13)
2. Replace file write error checks (lines 42, 125, 170)
3. Replace parse error checks (lines 51, 132, 178)
4. Replace assertion error checks (lines 61, 64, 136, 184)
5. Add edge case test function
6. Add package documentation

Best Practices from scratchpad/testing.md

• ✅ Use require.* for critical setup (file creation, parsing)
• ✅ Use assert.* for test validations (checking values, error messages)
• ✅ Write table-driven tests with t.Run() and descriptive names
• ✅ Always include helpful assertion messages
• ✅ No mocks needed - test real compiler interactions

Testing Commands

# Run tests for this file
go test -v ./pkg/workflow -run "TestInvalidReactionValue|TestNumericReactionParsing|TestInvalidNumericReaction"

# Run with coverage
go test -cover ./pkg/workflow -run "Reaction"

# Run all unit tests
make test-unit

Acceptance Criteria

• All 11 manual error checks replaced with testify assertions
• require.* used for setup (file writes, parsing)
• assert.* used for validations (value checks, error messages)
• All assertions include helpful messages
• Edge case tests added for "none", empty, whitespace, case
• Package documentation added explaining reaction testing
• 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
• Related Code:
  • pkg/workflow/compiler_safe_outputs.go - Reaction extraction and validation logic
  • pkg/workflow/compiler_types.go:423 - AIReaction field in WorkflowData struct
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (~1-2 hours)
Expected Impact: ✅ Improved test quality, better error messages, easier debugging, consistent codebase patterns

Files Involved:

• Test file: pkg/workflow/compiler_reactions_numeric_test.go
• Source files: pkg/workflow/compiler_safe_outputs.go, pkg/workflow/compiler_types.go

AI generated by Daily Testify Uber Super Expert

• expires on Feb 12, 2026, 11:34 AM UTC