Improve test quality: pkg/cli/health_command_test.go (#28622)

Author: Copilot

docs/adr/28622-test-validation-via-real-function-invocation.md

@@ -0,0 +1,65 @@
+# ADR-28622: Test Validation Logic via Real Function Invocation Rather Than Inline Re-implementation
+
+**Date**: 2026-04-26
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The `pkg/cli/health_command_test.go` test suite contained a `TestHealthConfigValidation` function that duplicated the days-validation logic inline rather than exercising the real `RunHealth` function. Additionally, `TestHealthCommand` used `assert.NotNil` for flag object lookups, which would cause a confusing nil-pointer panic if the assertion failed instead of stopping the test cleanly. Coverage of the JSON output path of `displayDetailedHealth` and of edge-case days values (0, -1, 91, 365) was absent. These gaps meant tests could pass even when the production validation path was broken.
+
+### Decision
+
+We will test the health command's validation behavior by calling `RunHealth` directly rather than re-implementing the validation predicate inside the test. We will use `require.NotNil` (and `require.NoError`) for any nil- or error-check whose failure would make subsequent assertions meaningless or panicky. New table-driven tests will cover all boundary values of the `days` parameter and the JSON output path of `displayDetailedHealth`.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep inline validation reimplementation in tests
+
+The existing approach checked `tt.config.Days != 7 && tt.config.Days != 30 && tt.config.Days != 90` directly in the test body. This keeps tests independent of `RunHealth` internals, but it duplicates the production validation predicate and will silently pass even if `RunHealth` loses its validation entirely, providing false confidence.
+
+#### Alternative 2: Introduce a mock or stub for RunHealth to isolate validation
+
+A dedicated `ValidateDays(days int) error` function could be extracted and tested independently without invoking `RunHealth`. This provides tighter unit isolation, but it requires production-code refactoring beyond the scope of improving existing test quality and postpones the coverage gains indefinitely.
+
+### Consequences
+
+#### Positive
+- Tests now exercise the actual `RunHealth` validation path; a regression in production validation will break tests immediately.
+- `require.NotNil` halts the test on nil rather than causing a confusing panic-in-assert, making failures easier to diagnose.
+- JSON output path of `displayDetailedHealth` gains explicit coverage, including the nil-runs and empty-runs boundary cases.
+
+#### Negative
+- Tests now depend on `RunHealth`'s exact error message format (`"invalid days value: %d"`, `"Must be 7, 30, or 90"`), coupling test assertions to production string literals that could change independently.
+- Valid-days test cases cannot assert a clean nil error because `RunHealth` may return a GitHub API error in CI environments without credentials; tests must tolerate that path.
+
+#### Neutral
+- The `require` package is introduced as an additional import alongside `assert`; both remain from the same `testify` library.
+- Test file line count increases significantly (37 deletions → 141 additions), which will register as a code-volume change in automated gates.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Test Implementation
+
+1. Tests for `RunHealth` **MUST** invoke `RunHealth` directly rather than re-implementing its validation predicates inline.
+2. Test assertions that dereference a pointer or use a value from a previous call **MUST** use `require.NotNil` or `require.NoError` (not `assert.NotNil` / `assert.NoError`) so the test halts before a nil-pointer panic can occur.
+3. Tests for `days` validation **MUST** cover at least the following boundary values: `0`, any negative value, `15` (a plausible but invalid value), `91`, and `365`, in addition to the three valid values `7`, `30`, `90`.
+4. Tests for `days` validation **MUST** assert that the returned error message contains both the invalid value (e.g., `"invalid days value: 0"`) and the set of valid options (e.g., `"Must be 7, 30, or 90"`).
+5. Tests that exercise JSON output paths (e.g., `displayDetailedHealth` with `JSONOutput: true`) **MUST** capture stdout, unmarshal the output as the declared JSON type, and assert at minimum that the top-level named fields match the input configuration.
+6. Tests **SHOULD NOT** assert a nil error for `RunHealth` calls with valid `days` values when the test environment may lack GitHub API credentials; instead they **SHOULD** assert only that the error (if any) does not contain `"invalid days value"`.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24966509091) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/health_command_test.go

@@ -3,84 +3,188 @@
 package cli
 
 import (
+	"encoding/json"
+	"io"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestHealthConfigValidation(t *testing.T) {
 	tests := []struct {
-		name      string
-		config    HealthConfig
-		shouldErr bool
+		name        string
+		config      HealthConfig
+		wantDaysErr bool
+		errContains string
 	}{
 		{
-			name: "valid 7 days",
-			config: HealthConfig{
-				Days:      7,
-				Threshold: 80.0,
-			},
-			shouldErr: false,
+			name:        "valid 7 days",
+			config:      HealthConfig{Days: 7, Threshold: 80.0},
+			wantDaysErr: false,
 		},
 		{
-			name: "valid 30 days",
-			config: HealthConfig{
-				Days:      30,
-				Threshold: 80.0,
-			},
-			shouldErr: false,
+			name:        "valid 30 days",
+			config:      HealthConfig{Days: 30, Threshold: 80.0},
+			wantDaysErr: false,
 		},
 		{
-			name: "valid 90 days",
-			config: HealthConfig{
-				Days:      90,
-				Threshold: 80.0,
-			},
-			shouldErr: false,
+			name:        "valid 90 days",
+			config:      HealthConfig{Days: 90, Threshold: 80.0},
+			wantDaysErr: false,
 		},
 		{
-			name: "invalid days value",
-			config: HealthConfig{
-				Days:      15,
-				Threshold: 80.0,
-			},
-			shouldErr: true,
+			name:        "zero days - validation error",
+			config:      HealthConfig{Days: 0, Threshold: 80.0},
+			wantDaysErr: true,
+			errContains: "invalid days value: 0",
+		},
+		{
+			name:        "negative days - validation error",
+			config:      HealthConfig{Days: -1, Threshold: 80.0},
+			wantDaysErr: true,
+			errContains: "invalid days value: -1",
+		},
+		{
+			name:        "days 15 - validation error",
+			config:      HealthConfig{Days: 15, Threshold: 80.0},
+			wantDaysErr: true,
+			errContains: "invalid days value: 15",
+		},
+		{
+			name:        "days 91 - validation error",
+			config:      HealthConfig{Days: 91, Threshold: 80.0},
+			wantDaysErr: true,
+			errContains: "invalid days value: 91",
+		},
+		{
+			name:        "days 365 - validation error",
+			config:      HealthConfig{Days: 365, Threshold: 80.0},
+			wantDaysErr: true,
+			errContains: "invalid days value: 365",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// For now, just validate the days parameter directly
-			// since the full RunHealth needs GitHub API access
-			if tt.config.Days != 7 && tt.config.Days != 30 && tt.config.Days != 90 {
-				assert.True(t, tt.shouldErr, "Should error for invalid days value")
+			err := RunHealth(tt.config)
+			if tt.wantDaysErr {
+				require.Error(t, err, "RunHealth should return a validation error for: %s", tt.name)
+				assert.Contains(t, err.Error(), tt.errContains, "Error message should describe the validation failure")
 			} else {
-				assert.False(t, tt.shouldErr, "Should not error for valid days value")
+				// Valid days values pass days validation; any error comes from GitHub API access
+				if err != nil {
+					assert.NotContains(t, err.Error(), "invalid days value", "Valid days should not produce a days validation error")
+				}
 			}
 		})
 	}
 }
 
+func TestRunHealthInvalidDays(t *testing.T) {
+	tests := []struct {
+		name        string
+		days        int
+		errContains string
+	}{
+		{name: "zero", days: 0, errContains: "invalid days value: 0"},
+		{name: "negative", days: -1, errContains: "invalid days value: -1"},
+		{name: "too large 91", days: 91, errContains: "invalid days value: 91"},
+		{name: "too large 365", days: 365, errContains: "invalid days value: 365"},
+		{name: "not a valid option 15", days: 15, errContains: "invalid days value: 15"},
+		{name: "not a valid option 8", days: 8, errContains: "invalid days value: 8"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			config := HealthConfig{Days: tt.days, Threshold: 80.0}
+			err := RunHealth(config)
+			require.Error(t, err, "RunHealth should return an error for days=%d", tt.days)
+			assert.Contains(t, err.Error(), tt.errContains, "Error should describe the invalid days value")
+			assert.Contains(t, err.Error(), "Must be 7, 30, or 90", "Error should list the valid days options")
+		})
+	}
+}
+
 func TestHealthCommand(t *testing.T) {
 	cmd := NewHealthCommand()
 
-	assert.NotNil(t, cmd, "Health command should be created")
+	require.NotNil(t, cmd, "Health command should be created")
 	assert.Equal(t, "health", cmd.Name(), "Command name should be 'health'")
 	assert.True(t, cmd.HasAvailableFlags(), "Command should have flags")
 	assert.Contains(t, cmd.Long, "Warnings when success rate drops below threshold", "Health help should consistently use warnings terminology")
 
 	// Check that required flags are registered
 	daysFlag := cmd.Flags().Lookup("days")
-	assert.NotNil(t, daysFlag, "Should have --days flag")
+	require.NotNil(t, daysFlag, "Should have --days flag")
 	assert.Equal(t, "7", daysFlag.DefValue, "Default days should be 7")
 
 	thresholdFlag := cmd.Flags().Lookup("threshold")
-	assert.NotNil(t, thresholdFlag, "Should have --threshold flag")
+	require.NotNil(t, thresholdFlag, "Should have --threshold flag")
 	assert.Equal(t, "80", thresholdFlag.DefValue, "Default threshold should be 80")
 
 	jsonFlag := cmd.Flags().Lookup("json")
-	assert.NotNil(t, jsonFlag, "Should have --json flag")
+	require.NotNil(t, jsonFlag, "Should have --json flag")
 
 	repoFlag := cmd.Flags().Lookup("repo")
-	assert.NotNil(t, repoFlag, "Should have --repo flag")
+	require.NotNil(t, repoFlag, "Should have --repo flag")
+}
+
+func TestDisplayDetailedHealthJSON(t *testing.T) {
+	tests := []struct {
+		name         string
+		runs         []WorkflowRun
+		wantZeroRuns bool
+	}{
+		{
+			name:         "nil runs - empty JSON structure",
+			runs:         nil,
+			wantZeroRuns: true,
+		},
+		{
+			name:         "empty runs slice - empty JSON structure",
+			runs:         []WorkflowRun{},
+			wantZeroRuns: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			config := HealthConfig{
+				WorkflowName: "test-workflow",
+				Days:         7,
+				Threshold:    80.0,
+				JSONOutput:   true,
+			}
+
+			// Redirect stdout to capture JSON output
+			oldStdout := os.Stdout
+			r, w, err := os.Pipe()
+			require.NoError(t, err, "os.Pipe should not fail")
+			os.Stdout = w
+
+			runErr := displayDetailedHealth(tt.runs, config)
+
+			w.Close()
+			os.Stdout = oldStdout
+
+			require.NoError(t, runErr, "displayDetailedHealth should not return an error for %s", tt.name)
+
+			outputBytes, readErr := io.ReadAll(r)
+			require.NoError(t, readErr, "Reading captured output should not fail")
+			output := string(outputBytes)
+
+			require.NotEmpty(t, output, "JSON output should not be empty")
+
+			var health WorkflowHealth
+			require.NoError(t, json.Unmarshal([]byte(output), &health), "Output should be valid JSON")
+
+			assert.Equal(t, "test-workflow", health.WorkflowName, "WorkflowName should match config")
+			if tt.wantZeroRuns {
+				assert.Equal(t, 0, health.TotalRuns, "TotalRuns should be zero for empty input")
+				assert.Equal(t, 0, health.SuccessCount, "SuccessCount should be zero for empty input")
+			}
+		})
+	}
 }