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

[github-actions]

The test file pkg/cli/health_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.

Current State

• Test File: pkg/cli/health_command_test.go
• Source File: pkg/cli/health_command.go
• Test Functions: 2 (TestHealthConfigValidation, TestHealthCommand)
• Lines of Code: 87 lines (test) / 343 lines (source)
• Exported Functions in Source: NewHealthCommand, RunHealth

Test Quality Analysis

Strengths ✅

1. Build tag present — correct //go:build !integration tag at the top
2. Table-driven test structure — TestHealthConfigValidation uses tests []struct with t.Run()
3. Assertion messages — most assertions include descriptive messages

🎯 Areas for Improvement

1. Circular Validation Logic (Critical Bug in Test Design)

TestHealthConfigValidation reimplements the validation logic inside the test body rather than calling RunHealth. The test effectively validates itself, not the production code. If someone changed the validation rules in RunHealth, this test would still pass.

Current (anti-pattern):

func TestHealthConfigValidation(t *testing.T) {
    // ...
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            // Reimplements production code logic here!
            if tt.config.Days != 7 && tt.config.Days != 30 && tt.config.Days != 90 {
                assert.True(t, tt.shouldErr, "Should error for invalid days value")
            } else {
                assert.False(t, tt.shouldErr, "Should not error for valid days value")
            }
        })
    }
}

Recommended (calls actual production function):

func TestRunHealthValidation(t *testing.T) {
    tests := []struct {
        name      string
        days      int
        shouldErr bool
        errMsg    string
    }{
        {name: "valid 7 days", days: 7, shouldErr: false},
        {name: "valid 30 days", days: 30, shouldErr: false},
        {name: "valid 90 days", days: 90, shouldErr: false},
        {name: "invalid days 15", days: 15, shouldErr: true, errMsg: "invalid days value: 15"},
        {name: "invalid days 0", days: 0, shouldErr: true, errMsg: "invalid days value: 0"},
        {name: "invalid days 365", days: 365, shouldErr: true, errMsg: "invalid days value: 365"},
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            config := HealthConfig{
                Days:      tt.days,
                Threshold: 80.0,
                // WorkflowName empty so it short-circuits before GitHub API call
            }
            err := RunHealth(config)
            // NOTE: RunHealth will fail after validation with a GitHub API error;
            // we only care that invalid days returns the validation error.
            if tt.shouldErr {
                require.Error(t, err, "RunHealth should return error for invalid days")
                assert.Contains(t, err.Error(), tt.errMsg, "Error message should describe the invalid value")
            }
            // Valid days will proceed past validation and fail on GitHub API — that's OK
        })
    }
}

Why this matters: The current test provides no real coverage guarantee. If RunHealth's validation logic were deleted entirely, the test would still pass.

2. Missing require.NotNil Before Dereferencing Command

TestHealthCommand calls cmd.Name(), cmd.HasAvailableFlags(), etc. immediately after the assert.NotNil. If cmd were ever nil, the test would panic with a nil-pointer dereference rather than producing a clear failure.

Current:

cmd := NewHealthCommand()
assert.NotNil(t, cmd, "Health command should be created")
assert.Equal(t, "health", cmd.Name(), ...)  // panics if cmd is nil

Recommended:

cmd := NewHealthCommand()
require.NotNil(t, cmd, "Health command should be created")  // stops test on nil
assert.Equal(t, "health", cmd.Name(), "Command name should be 'health'")

3. Missing Edge Cases for Validation

The test covers valid values (7, 30, 90) and one invalid value (15), but misses important edge cases:

• Days = 0 (zero value / unset)
• Days = -1 (negative values)
• Days = 91 (just above a valid value)
• Threshold = 0.0 (zero threshold)
• Threshold = 100.0 (maximum valid threshold)
• Threshold = -1.0 (negative threshold — currently no validation in source)

4. Missing Tests for Key Functions

The source file has several functions that are fully testable without GitHub API access:

displayDetailedHealth with empty runs (JSON output path):

func TestDisplayDetailedHealthJSON(t *testing.T) {
    config := HealthConfig{
        WorkflowName: "my-workflow",
        Days:         7,
        Threshold:    80.0,
        JSONOutput:   true,
    }
    // Capture stdout
    old := os.Stdout
    r, w, _ := os.Pipe()
    os.Stdout = w

    err := displayDetailedHealth(nil, config)
    
    w.Close()
    os.Stdout = old
    var buf bytes.Buffer
    _, _ = io.Copy(&buf, r)

    require.NoError(t, err, "displayDetailedHealth should not error with nil runs")
    assert.True(t, json.Valid(buf.Bytes()), "Output should be valid JSON")
}

outputHealthJSON with valid summary:

func TestOutputHealthJSON(t *testing.T) {
    summary := HealthSummary{
        Period:         "Last 7 Days",
        BelowThreshold: 0,
        Workflows:      []WorkflowHealth{},
    }
    // Verify no error on marshaling
    err := outputHealthJSON(summary)
    require.NoError(t, err, "outputHealthJSON should succeed with valid summary")
}

RunHealth days validation (without needing GitHub API):
Since RunHealth returns early for invalid Days before making any API calls, the error path is fully testable:

func TestRunHealthInvalidDays(t *testing.T) {
    for _, days := range []int{0, 1, 15, 31, 91, 365, -1} {
        t.Run(fmt.Sprintf("days=%d", days), func(t *testing.T) {
            err := RunHealth(HealthConfig{Days: days, Threshold: 80.0})
            require.Error(t, err, "RunHealth should fail for days=%d", days)
            assert.Contains(t, err.Error(), "invalid days value", "Error should mention invalid days")
        })
    }
}

5. Default Flag Values Should Use require for Flag Lookup

// Current
daysFlag := cmd.Flags().Lookup("days")
assert.NotNil(t, daysFlag, "Should have --days flag")
assert.Equal(t, "7", daysFlag.DefValue, ...)  // panics if daysFlag is nil

// Recommended
daysFlag := cmd.Flags().Lookup("days")
require.NotNil(t, daysFlag, "Should have --days flag")
assert.Equal(t, "7", daysFlag.DefValue, "Default days should be 7")

Apply to all three flag lookups: days, threshold, json, repo.

📋 Implementation Guidelines

Priority Order

1. High: Fix TestHealthConfigValidation to call RunHealth directly for the invalid-days error path
2. High: Change assert.NotNil → require.NotNil for cmd and all flag lookups
3. Medium: Add TestRunHealthInvalidDays covering 0, negative, and out-of-range values
4. Medium: Add TestDisplayDetailedHealthJSON for the nil-runs JSON output path
5. Low: Add TestOutputHealthJSON and TestOutputHealthTable for the output helpers

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
• ✅ Always include helpful assertion messages

Testing Commands

go test -v -run "TestHealth" ./pkg/cli/
go test -v -run "TestRunHealth" ./pkg/cli/
make test-unit

Acceptance Criteria

• TestHealthConfigValidation refactored to call RunHealth for the validation path
• require.NotNil used for command and flag objects before dereferencing
• Edge-case days values (0, -1, 91, 365) covered by tests
• TestRunHealthInvalidDays table-driven test added
• TestDisplayDetailedHealthJSON added for nil-runs + JSON output path
• All tests pass: go test -v -run "TestHealth" ./pkg/cli/
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example Tests: pkg/cli/add_command_test.go uses require.NotNil + table-driven structure as a good reference
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (2–3 hours)
Expected Impact: Real validation coverage, safer test execution, clearer failure messages

Files Involved:

• Test file: pkg/cli/health_command_test.go
• Source file: pkg/cli/health_command.go

Generated by Daily Testify Uber Super Expert · ● 967K · ◷

• expires on Apr 28, 2026, 11:35 AM UTC

[github-actions]

🍪 *Issue Monster selected this for Copilotecho BEGIN___COMMAND_OUTPUT_MARKER ; PS1= ; PS2= ; unset HISTFILE ; EC=0 ; echo ___BEGIN___COMMAND_DONE_MARKER___0 ; }

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! 🍪

Caution

Security scanning requires review for Issue Monster

Details

Potential security threats were detected in the agent output. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

🍪 Om nom nom by Issue Monster · ● 23K · ◷