From 0934333b02e2b78efc09fbe6e364948ea7f05c33 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:27:37 +0000 Subject: [PATCH 1/3] Initial plan From 89f2aa807b9472d4f392c0cfdbcc657f9e726b83 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:42:11 +0000 Subject: [PATCH 2/3] improve test quality: pkg/cli/health_command_test.go - Refactor TestHealthConfigValidation to call RunHealth for validation path - Add edge-case days values (0, -1, 91, 365) to TestHealthConfigValidation - Use require.NotNil for command/flag objects in TestHealthCommand - Add TestRunHealthInvalidDays table-driven test - Add TestDisplayDetailedHealthJSON for nil-runs + JSON output path - Fix io.ReadAll usage to properly handle read errors" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6427f696-4261-41e1-a561-761885b30c1a Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/health_command_test.go | 178 ++++++++++++++++++++++++++------- 1 file changed, 141 insertions(+), 37 deletions(-) diff --git a/pkg/cli/health_command_test.go b/pkg/cli/health_command_test.go index c8760bf0158..3a3cbb975c6 100644 --- a/pkg/cli/health_command_test.go +++ b/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") + } + }) + } } From 88f5a82e610a66d910006f819653603b3b886e89 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 26 Apr 2026 20:44:39 +0000 Subject: [PATCH 3/3] docs(adr): add draft ADR-28622 for test validation strategy Add draft Architecture Decision Record documenting the decision to test validation logic via real RunHealth function invocation rather than inline re-implementation in health_command_test.go. Co-Authored-By: Claude Sonnet 4.6 --- ...validation-via-real-function-invocation.md | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 docs/adr/28622-test-validation-via-real-function-invocation.md diff --git a/docs/adr/28622-test-validation-via-real-function-invocation.md b/docs/adr/28622-test-validation-via-real-function-invocation.md new file mode 100644 index 00000000000..7aa115aa648 --- /dev/null +++ b/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.*