[testify-expert] Improve Test Quality: pkg/cli/add_command_test.go

[agentic-workflows-dev]

Overview

The test file pkg/cli/add_command_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/cli/add_command_test.go
• Source File: pkg/cli/add_command.go
• Test Functions: 7 test functions
• Lines of Code: 208 lines
• Assertions: 38 testify assertions
• Last Modified: 2026-01-29

Test Quality Analysis

Strengths ✅

1. Excellent testify adoption - The file consistently uses assert.* and require.* throughout
2. Good use of table-driven tests - Tests like TestAddWorkflows_InvalidNumber and TestAddCommandFlagDefaults follow table-driven patterns
3. Comprehensive flag testing - Thorough validation of command flags, shorthands, and defaults

Areas for Improvement 🎯

1. Missing Test Coverage for Core Functions

Current Gap: The source file contains 3 exported functions, but only 2 are tested:

• ✅ NewAddCommand - TESTED
• ✅ AddWorkflows - PARTIALLY TESTED (only error cases)
• ❌ AddResolvedWorkflows - NOT TESTED

Impact: AddResolvedWorkflows is a critical function that handles the actual workflow addition logic after resolution. This represents a significant coverage gap.

Recommended Test Cases:

View Proposed Test Implementation

func TestAddResolvedWorkflows(t *testing.T) {
    tests := []struct {
        name           string
        workflowStrings []string
        resolved       *ResolvedWorkflows
        number         int
        shouldErr      bool
        errorContains  string
    }{
        {
            name:           "nil resolved workflows",
            workflowStrings: []string{"test-workflow"},
            resolved:       nil,
            number:         1,
            shouldErr:      true,
            errorContains:  "resolved",
        },
        {
            name:           "empty resolved workflows",
            workflowStrings: []string{"test-workflow"},
            resolved:       &ResolvedWorkflows{Workflows: []*ResolvedWorkflow{}},
            number:         1,
            shouldErr:      true,
            errorContains:  "no workflows",
        },
        {
            name:           "invalid number zero",
            workflowStrings: []string{"test-workflow"},
            resolved: &ResolvedWorkflows{
                Workflows: []*ResolvedWorkflow{
                    {Spec: &WorkflowSpec{WorkflowName: "test"}},
                },
            },
            number:         0,
            shouldErr:      true,
            errorContains:  "number",
        },
        {
            name:           "invalid number negative",
            workflowStrings: []string{"test-workflow"},
            resolved: &ResolvedWorkflows{
                Workflows: []*ResolvedWorkflow{
                    {Spec: &WorkflowSpec{WorkflowName: "test"}},
                },
            },
            number:         -1,
            shouldErr:      true,
            errorContains:  "number",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := AddResolvedWorkflows(
                tt.workflowStrings,
                tt.resolved,
                tt.number,
                false, // verbose
                false, // quiet
                "",    // engineOverride
                "",    // name
                false, // force
                "",    // appendText
                false, // createPR
                false, // push
                false, // noGitattributes
                "",    // workflowDir
                false, // noStopAfter
                "",    // stopAfter
            )

            if tt.shouldErr {
                require.Error(t, err, "Should return error for: %s", tt.name)
                if tt.errorContains != "" {
                    assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected text")
                }
                assert.Nil(t, result, "Result should be nil on error")
            } else {
                require.NoError(t, err, "Should not return error for: %s", tt.name)
                assert.NotNil(t, result, "Result should not be nil on success")
            }
        })
    }
}

Why this matters: Testing the full function chain ensures that workflow addition logic works correctly end-to-end, not just in error cases.

2. Incomplete Test for AddWorkflows Success Cases

Current Issue: TestAddWorkflows_EmptyWorkflows only tests the error case. The success path of AddWorkflows is not tested.

Recommended Changes:

View Enhanced Test Implementation

func TestAddWorkflows(t *testing.T) {
    tests := []struct {
        name        string
        workflows   []string
        number      int
        shouldErr   bool
        errorContains string
    }{
        {
            name:          "empty workflows",
            workflows:     []string{},
            number:        1,
            shouldErr:     true,
            errorContains: "at least one workflow",
        },
        {
            name:          "invalid number zero",
            workflows:     []string{"test-workflow"},
            number:        0,
            shouldErr:     true,
            errorContains: "number",
        },
        {
            name:          "invalid number negative",
            workflows:     []string{"test-workflow"},
            number:        -1,
            shouldErr:     true,
            errorContains: "number",
        },
        // Note: Success cases may require integration test or mock setup
        // Consider adding integration tests in a separate file
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := AddWorkflows(
                tt.workflows,
                tt.number,
                false, // verbose
                "",    // engineOverride
                "",    // name
                false, // force
                "",    // appendText
                false, // createPR
                false, // push
                false, // noGitattributes
                "",    // workflowDir
                false, // noStopAfter
                "",    // stopAfter
            )

            if tt.shouldErr {
                require.Error(t, err, "Should return error for: %s", tt.name)
                if tt.errorContains != "" {
                    assert.Contains(t, err.Error(), tt.errorContains, "Error should mention: %s", tt.errorContains)
                }
                assert.Nil(t, result, "Result should be nil when error occurs")
            } else {
                require.NoError(t, err, "Should not return error for: %s", tt.name)
                assert.NotNil(t, result, "Result should not be nil on success")
            }
        })
    }
}

Why this matters: Consolidating related tests into a single table-driven test makes it easier to add new test cases and see all scenarios at a glance.

3. Redundant Test: TestAddWorkflows_InvalidNumber

Current Issue: TestAddWorkflows_InvalidNumber is a table-driven test but doesn't actually call AddWorkflows. The test body only validates that the number is non-positive without exercising the actual function.

Current Code (lines 92-126):

func TestAddWorkflows_InvalidNumber(t *testing.T) {
    // ... table setup ...
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            // Note: This test checks validation logic exists
            // Full integration would require actual workflow specs
            if tt.expectError && tt.number <= 0 {
                // The function should validate number > 0
                // This is a placeholder - actual validation may occur in the function
                assert.LessOrEqual(t, tt.number, 0, "Invalid number should be non-positive")
            }
        })
    }
}

Recommended Change: Remove this test and consolidate the number validation cases into TestAddWorkflows (see improvement #2 above).

Why this matters: Having a test that doesn't actually test the function is misleading and provides false confidence in test coverage.

4. Placeholder Test: TestAddCommandStructure

Current Issue: TestAddCommandStructure is essentially a placeholder that only verifies the command is not nil (lines 131-150).

Current Code:

func TestAddCommandStructure(t *testing.T) {
    tests := []struct {
        name           string
        commandCreator func() interface{}
    }{
        {
            name: "add command exists",
            commandCreator: func() interface{} {
                return NewAddCommand(validateEngineStub)
            },
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            cmd := tt.commandCreator()
            require.NotNil(t, cmd, "Command should not be nil")
        })
    }
}

Recommended Change: Remove this test as it duplicates the nil check already performed in TestNewAddCommand (line 20).

Why this matters: Duplicate tests add maintenance burden without improving coverage. The table-driven structure adds unnecessary complexity for a single trivial test.

5. Missing Edge Case Tests

Current Gaps: Several edge cases are not tested:

1. Flag interaction conflicts - What happens when conflicting flags are used together?
2. Wildcard workflow handling - No tests for wildcard pattern behavior
3. PR creation prerequisites - Tests for GitHub CLI availability, git repo checks
4. Directory creation - Tests for custom --dir subdirectory creation
5. Stop-after behavior - No tests for --stop-after and --no-stop-after interaction

Recommended Test Cases:

View Edge Case Tests

func TestAddCommandFlagInteractions(t *testing.T) {
    tests := []struct {
        name          string
        flags         map[string]interface{}
        shouldWarn    bool
        warningText   string
    }{
        {
            name: "both stop-after flags",
            flags: map[string]interface{}{
                "stop-after":    "compile",
                "no-stop-after": true,
            },
            shouldWarn:  true,
            warningText: "conflicting",
        },
        {
            name: "PR without push",
            flags: map[string]interface{}{
                "create-pull-request": true,
                "push":                false,
            },
            shouldWarn: false, // This is valid - PR creation may handle push
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            cmd := NewAddCommand(validateEngineStub)
            
            // Set flags
            for flagName, value := range tt.flags {
                switch v := value.(type) {
                case bool:
                    require.NoError(t, cmd.Flags().Set(flagName, fmt.Sprintf("%t", v)))
                case string:
                    require.NoError(t, cmd.Flags().Set(flagName, v))
                case int:
                    require.NoError(t, cmd.Flags().Set(flagName, fmt.Sprintf("%d", v)))
                }
            }
            
            // Test flag interactions
            // Note: This may require adding validation logic to the command
            // For now, just verify flags can be set
            for flagName := range tt.flags {
                flag := cmd.Flags().Lookup(flagName)
                assert.NotNil(t, flag, "Flag should exist: %s", flagName)
            }
        })
    }
}

func TestAddWorkflowsResultStruct(t *testing.T) {
    // Test the result struct
    result := &AddWorkflowsResult{
        PRNumber:            42,
        PRURL:              "https://github.com/owner/repo/pull/42",
        HasWorkflowDispatch: true,
    }
    
    assert.Equal(t, 42, result.PRNumber, "PR number should be set")
    assert.Equal(t, "https://github.com/owner/repo/pull/42", result.PRURL, "PR URL should be set")
    assert.True(t, result.HasWorkflowDispatch, "HasWorkflowDispatch should be true")
    
    // Test zero values
    emptyResult := &AddWorkflowsResult{}
    assert.Equal(t, 0, emptyResult.PRNumber, "Default PR number should be 0")
    assert.Empty(t, emptyResult.PRURL, "Default PR URL should be empty")
    assert.False(t, emptyResult.HasWorkflowDispatch, "Default HasWorkflowDispatch should be false")
}

Why this matters: Edge cases are where bugs often hide. Testing flag interactions and struct behavior ensures robust error handling.

Implementation Guidelines

Priority Order

1. High Priority - Remove placeholder/redundant tests (TestAddCommandStructure, TestAddWorkflows_InvalidNumber)
2. High Priority - Consolidate AddWorkflows tests into comprehensive table-driven test
3. High Priority - Add tests for AddResolvedWorkflows function
4. Medium Priority - Add edge case tests for flag interactions
5. Medium Priority - Add tests for AddWorkflowsResult struct behavior
6. Low Priority - Add integration tests for success paths (may require separate integration test file)

Best Practices from specs/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 tests for this file
go test -v ./pkg/cli -run "TestAdd|TestNew.*Add"

# Run specific test
go test -v ./pkg/cli -run TestAddWorkflows

# Run tests with coverage
go test -cover ./pkg/cli -run "TestAdd"

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

# Run full test suite
make test-unit

Acceptance Criteria

• Remove redundant/placeholder tests (TestAddCommandStructure, TestAddWorkflows_InvalidNumber)
• Consolidate AddWorkflows error cases into single table-driven test with all scenarios
• Add comprehensive tests for AddResolvedWorkflows function (error cases at minimum)
• Add tests for AddWorkflowsResult struct initialization and field access
• Add edge case tests for flag interactions
• All test names are descriptive and follow conventions
• All assertions include helpful messages
• Tests pass: make test-unit
• Code follows patterns in specs/testing.md

Additional Context

• Repository Testing Guidelines: See specs/testing.md for comprehensive testing patterns
• Example Tests: Look at recent test files in pkg/workflow/*_test.go for table-driven test examples
• Testify Documentation: https://github.com/stretchr/testify

Summary of Changes

Remove (2 tests):

• TestAddCommandStructure - Redundant nil check
• TestAddWorkflows_InvalidNumber - Placeholder that doesn't call the function

Enhance (2 tests):

• TestAddWorkflows_EmptyWorkflows → TestAddWorkflows - Consolidate error cases
• Add TestAddResolvedWorkflows - Cover missing exported function

Add (2 new test areas):

• TestAddCommandFlagInteractions - Edge cases for flag conflicts
• TestAddWorkflowsResultStruct - Struct behavior validation

Expected outcome: Improved coverage from ~60% to ~85%+ for exported functions, with cleaner test organization and no placeholder tests.

---

Priority: Medium
Effort: Medium (3-4 hours estimated)
Expected Impact: Significantly improved test coverage and confidence in workflow addition logic

Files Involved:

• Test file: pkg/cli/add_command_test.go
• Source file: pkg/cli/add_command.go

AI generated by Daily Testify Uber Super Expert

[github-actions]

🍪 Issue Monster has assigned this to Copilot!

I've identified this issue as a good candidate for automated resolution and assigned it to the Copilot agent.

The Copilot agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

AI generated by Issue Monster