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

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

<details>
<summary>🎯 Areas for Improvement</summary>

#### 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):**
```go
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):**
```go
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:**
```go
cmd := NewHealthCommand()
assert.NotNil(t, cmd, "Health command should be created")
assert.Equal(t, "health", cmd.Name(), ...)  // panics if cmd is nil
```

**Recommended:**
```go
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):**
```go
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:**
```go
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:
```go
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

```go
// 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`.

</details>

<details>
<summary>📋 Implementation Guidelines</summary>

#### 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
```bash
go test -v -run "TestHealth" ./pkg/cli/
go test -v -run "TestRunHealth" ./pkg/cli/
make test-unit
```

</details>

### 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](https://github.com/github/gh-aw/actions/runs/24955529195/agentic_workflow) · ● 967K · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw+is%3Aissue+%22gh-aw-workflow-call-id%3A+github%2Fgh-aw%2Fdaily-testify-uber-super-expert%22&type=issues)
> - [x] expires  on Apr 28, 2026, 11:35 AM UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Current State

Test Quality Analysis

Strengths ✅

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

2. Missing `require.NotNil` Before Dereferencing Command

3. Missing Edge Cases for Validation

4. Missing Tests for Key Functions

5. Default Flag Values Should Use `require` for Flag Lookup

Priority Order

Best Practices from `scratchpad/testing.md`

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

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

Description

Current State

Test Quality Analysis

Strengths ✅

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

2. Missing require.NotNil Before Dereferencing Command

3. Missing Edge Cases for Validation

4. Missing Tests for Key Functions

5. Default Flag Values Should Use require for Flag Lookup

Priority Order

Best Practices from scratchpad/testing.md

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

2. Missing `require.NotNil` Before Dereferencing Command

5. Default Flag Values Should Use `require` for Flag Lookup

Best Practices from `scratchpad/testing.md`