[ci-coach] fix: mock GitHub API in TestHealthConfigValidation to prevent pkg/cli test timeout - #55411
Conversation
…timeout TestHealthConfigValidation's 'valid days' subtests called RunHealth without mocking healthListWorkflowRuns, so each fell through to real GitHub API pagination (up to MaxIterations batches). This took 46-48s per subtest (141s total) in CI, pushing the whole pkg/cli unit test binary (which has a 3m timeout in the ci-coach validation step) over budget and causing 'panic: test timed out after 3m0s'. Stub healthListWorkflowRuns to return no runs immediately, since this test only exercises the Days validation branch in RunHealth, not GitHub API pagination (which is already covered by TestFetchWorkflowRunsPaginatesPastFilteredBatches and friends). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 10 new lines of code in business logic directories (threshold is 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Pull request overview
Prevents health validation tests from making real GitHub API requests.
Changes:
- Stubs workflow-run listing during
TestHealthConfigValidation. - Restores the original implementation through test cleanup.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/health_command_test.go |
Adds deterministic API mocking for validation tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — approving. The fix is minimal, correctly targeted, and follows the established mocking pattern in the package.
📋 Summary
Positive Highlights
- ✅ Root cause properly addressed: valid-days subtests no longer hit the real GitHub API
- ✅
t.Cleanupensures the stub is torn down even if a subtest panics — good hygiene - ✅ Stub is placed before the
forloop, covering all subtests consistently - ✅ Comment clearly explains the scope boundary (days validation only; run listing covered elsewhere)
- ✅ Pattern mirrors existing
healthListWorkflowRunsusage in neighboring tests
No regression risk: error/validation subtests return before reaching the stub, so the mock is harmless there.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 13.7 AIC · ⌖ 9.72 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with Impeccable audit mode (tests-only change).
The mock is correctly scoped: t.Cleanup restores the original after all sub-tests run, and the stub returns nil, 0, nil so valid-days cases do not fall through to real network access. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 15.2 AIC · ⌖ 8.96 AIC · ⊞ 6.2K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
Non-blocking change; this correctly removes an accidental network dependency from a unit test without introducing an obvious regression in the touched lines.
Notes
I only reviewed pkg/cli/health_command_test.go. The new package-level stub is restored with t.Cleanup, keeps the test focused on days validation, and matches the existing test seam already used elsewhere in this package. I did not find a changed-line issue that warrants blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 3.74 AIC · ⌖ 6.79 AIC · ⊞ 4.6K
Comment /review to run again
Summary
TestHealthConfigValidationinpkg/cli/health_command_test.gopreviously fell through to a real GitHub API call (healthListWorkflowRuns) for its "valid days" test cases, which could take tens of seconds or hang in sandboxed/offline CI environments and causedpkg/clitest timeouts. The fix stubs this call with a mock that returns an empty result, since the test only validates thedaysparameter and run-listing behavior is covered elsewhere.Change Classification
Key Changes
pkg/cli/health_command_test.gohealthListWorkflowRunswith a mock returning(nil, 0, nil), restored viat.Cleanup, before runningTestHealthConfigValidationcasesImpact Assessment
pkg/clitest suite reliability and speed in sandboxed/offline environments by eliminating unintended network calls.Commits