Improve test quality in step_types_test.go: convert manual assertions to testify

[Copilot]

Converts 20+ manual error checks to testify assertions and adds edge case coverage in pkg/workflow/step_types_test.go.

Changes

Manual assertions → testify:

• Replaced t.Errorf() with assert.*() for validations
• Replaced t.Fatalf() with require.*() for setup steps
• Added descriptive messages to all assertions

Edge case tests added:

• TestMapToStep_InvalidTypes - non-integer timeout-minutes, string expressions, non-string env values
• TestClone_DeepNestedMaps - validates shallow copy behavior with nested maps
• TestToYAML_ErrorHandling - complex nested structures, special characters, multiline strings
• TestSliceToSteps_MixedValidInvalid - invalid step types, empty maps

Linting fixes:

• Used assert.Len() for length comparisons
• Corrected assert.NotEqual() argument order

Example

Before:

if got := tt.step.IsUsesStep(); got != tt.want {
    t.Errorf("WorkflowStep.IsUsesStep() = %v, want %v", got, tt.want)
}

After:

got := tt.step.IsUsesStep()
assert.Equal(t, tt.want, got, "IsUsesStep should return correct value for %s", tt.name)

Test coverage: 15 functions (11 improved, 4 new edge cases)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[testify-expert] Improve Test Quality: pkg/workflow/step_types_test.go</issue_title>
<issue_description>The test file pkg/workflow/step_types_test.go demonstrates excellent testing practices but has opportunities for improvement. This issue provides specific recommendations to enhance test quality using testify best practices.

Current State

• Test File: pkg/workflow/step_types_test.go
• Source File: pkg/workflow/step_types.go
• Test Functions: 11 test functions
• Lines of Code: 740 lines
• Test Coverage: Excellent - all public functions have tests

Strengths ✅

1. Excellent table-driven test structure - All major functions use table-driven tests
2. Comprehensive coverage - Tests cover all 8 public functions
3. Good test organization - Helper functions are well-designed
4. Round-trip testing - Includes bidirectional conversion tests

Areas for Improvement 🎯

1. Convert Manual Assertions to Testify

Lines to update: 32-34, 64-66, 159-161, 273-275, 305-307, 310-313, 315-318, 320-323, 361-363, 365-367, 385-386, 391-393, 397-400

// ❌ CURRENT (lines 32-34)
if got := tt.step.IsUsesStep(); got != tt.want {
    t.Errorf("WorkflowStep.IsUsesStep() = %v, want %v", got, tt.want)
}

// ✅ IMPROVED
got := tt.step.IsUsesStep()
assert.Equal(t, tt.want, got, "IsUsesStep should return correct value for %s", tt.name)

// ❌ CURRENT (lines 273-275)
if (err != nil) != tt.wantErr {
    t.Errorf("MapToStep() error = %v, wantErr %v", err, tt.wantErr)
    return
}

// ✅ IMPROVED
if tt.wantErr {
    require.Error(t, err, "MapToStep should return error for %s", tt.name)
    return
} else {
    require.NoError(t, err, "MapToStep should succeed for %s", tt.name)
}

// ❌ CURRENT (lines 385-386)
if err != nil {
    t.Fatalf("MapToStep() failed: %v", err)
}

// ✅ IMPROVED
require.NoError(t, err, "MapToStep should succeed for round-trip test")

2. Add Assertion Messages

All assertions should include descriptive messages:

// ❌ CURRENT
assert.Equal(t, expected, result)

// ✅ IMPROVED
assert.Equal(t, expected, result, "ToMap should correctly convert %s field", fieldName)
assert.NoError(t, err, "MapToStep should successfully parse valid step configuration")
assert.True(t, step.IsUsesStep(), "Step with 'uses' field should be identified as uses step")

3. Add Edge Case Tests

Missing tests:

1. Invalid type conversions in MapToStep
2. Deep nested map mutations in Clone
3. Error handling in ToYAML
4. Mixed valid/invalid steps in SliceToSteps

func TestMapToStep_InvalidTypes(t *testing.T) {
    tests := []struct {
        name    string
        stepMap map[string]any
        wantErr bool
    }{
        {
            name: "timeout-minutes as string",
            stepMap: map[string]any{
                "name":            "Test",
                "timeout-minutes": "not-a-number",
            },
            wantErr: false, // Currently ignores invalid types
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            step, err := MapToStep(tt.stepMap)
            if tt.wantErr {
                require.Error(t, err)
            } else {
                require.NoError(t, err)
                require.NotNil(t, step)
            }
        })
    }
}

func TestClone_DeepNestedMaps(t *testing.T) {
    original := &WorkflowStep{
        Name: "Test",
        With: map[string]any{
            "level1": map[string]any{
                "level2": map[string]any{"level3": "value"},
            },
        },
    }

    clone := original.Clone()
    
    // Modify deeply nested value
    level1 := clone.With["level1"].(map[string]any)
    level2 := level1["level2"].(map[string]any)
    level2["level3"] = "modified"
    
    // Verify original unchanged
    origLevel1 := original.With["level1"].(map[string]any)
    origLevel2 := origLevel1["level2"].(map[string]any)
    assert.Equal(t, "value", origLevel2["level3"], "Clone should deep copy nested maps")
}

Implementation Priority

1. High: Convert manual error checks to testify (20+ locations)
2. High: Add assertion messages
3. Medium: Add edge case tests
4. Low: Add package comment

Best Practices from specs/testing.md

• ✅ Use require.* for critical setup
• ✅ Use assert.* for test validations
• ✅ Write table-driven tests with t.Run()
• ✅ Always include helpful assertion messages
• ❌ Currently using manual t.Errorf in many places

Testing Commands

# Run tests for this file
go test -v ./pkg/workflow -run TestWorkflowStep

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

# Run all unit tests
make test-unit

Acceptance Criteria

• All manual checks replaced with testify assertions
• All assertions include helpful messages
• Edge case tests added for invalid types and deep nes...

• Fixes [testify-expert] Improve Test Quality: pkg/workflow/step_types_test.go #10054

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.