Skip to content

Fix #894: mock fetchIssue in next.test.ts to eliminate gh-CLI flake#896

Merged
amrmelsayed merged 6 commits into
mainfrom
builder/bugfix-894
May 28, 2026
Merged

Fix #894: mock fetchIssue in next.test.ts to eliminate gh-CLI flake#896
amrmelsayed merged 6 commits into
mainfrom
builder/bugfix-894

Conversation

@amrmelsayed
Copy link
Copy Markdown
Collaborator

Summary

The emits BUILD tasks for fresh specify phase test (and ~9 other tests in next.test.ts that hit the build path) intermittently timed out at vitest's default 5s on CI. Root cause was a real gh issue view subprocess on every next() call. This PR mocks fetchIssue to make the suite deterministic and ~40x faster.

Fixes #894

Root Cause

next()buildPhasePrompt()getProjectSummary()fetchIssue() shells out to gh issue view <id> via executeForgeCommand. Every test exercising the build path therefore pays one gh+network round-trip. Locally each call is ~900ms; on CI under variable load it can spike past 5s — vitest's default per-test timeout — and any of the 10 affected tests is equally at risk on a slow run. The failing test in PR #893's CI happened to be the first to cross the line.

This matches hypothesis #2 from the issue's fix-options table ("subprocess that should be mocked").

Fix

Added a single vi.mock('../../../lib/github.js', …) block (8 lines including comment) to next.test.ts, returning null from fetchIssue. This is the same pattern already used in src/__tests__/project-summary.test.ts. Tests fall through to the existing spec-file / status-title fallbacks — the same path they implicitly took before, since fixtures don't create real GitHub issues.

Test Plan

  • Build passes (pnpm build)
  • All 30 tests in next.test.ts still pass
  • All 334 tests across src/commands/porch/__tests__/ still pass
  • File duration drops from ~11s → ~258ms; per-test build-path times drop from 836-2000ms → 2-8ms (even a 100x CI slowdown stays well under 5s)
  • No follow-up "test genuinely slow" issue needed — the fix is a proper mock, not a timeout bump, so there's no real performance bloat being papered over

Verification across CI runs

After this lands, the test should pass consistently. If the flake recurs, the cause is something other than fetchIssue and would need fresh investigation.

The 'emits BUILD tasks for fresh specify phase' test (and ~9 other tests
in next.test.ts that hit the build path) was timing out at 5s on CI.
Root cause: next() → buildPhasePrompt() → getProjectSummary() →
fetchIssue() shells out to `gh issue view <id>` on every call. Each
gh+network round-trip is ~900ms locally and can spike past vitest's
default 5s per-test timeout on CI under variable load.

Fix: add vi.mock('../../../lib/github.js') returning null from
fetchIssue, mirroring the established pattern in
src/__tests__/project-summary.test.ts. Tests fall through to the
existing spec-file / status-title fallbacks, which is what the
test fixtures already implicitly relied on.

Result: file duration drops from ~11s to ~258ms; per-test times for
build-path tests drop from 836-2000ms to 2-8ms. All 30 tests still pass.
@amrmelsayed
Copy link
Copy Markdown
Collaborator Author

Architect Review

Low-risk, root-cause fix. 56/0 across 3 files (test file + 2 bookkeeping). No production code modified — pure test infrastructure. CMAP unanimous APPROVE.

Verified

  • Root cause correctly identified. next()buildPhasePrompt()getProjectSummary()fetchIssue() shells out to gh issue view <id>. Every test exercising the build path paid one gh subprocess + network round-trip, ~900ms locally, multi-second on CI under load. Vitest's 5s default timeout was the boundary — that's exactly hypothesis Context Window Optimization - AI Needs Project Knowledge Without Breaking Limits #2 from the issue body's fix-options table
  • Proper fix, not a band-aid. 8-line vi.mock('../../../lib/github.js', …) block returning null from fetchIssue. Tests fall through to existing spec-file / status-title fallback paths — same path they implicitly took before, since fixtures never created real GitHub issues. The mock just removes the dependency that was always unnecessary at this layer
  • Matches existing precedent — same vi.mock pattern is already used in src/__tests__/project-summary.test.ts, so this is following the established testing convention for this codebase rather than introducing a new pattern
  • Quantified win. File runtime: ~11s → ~258ms (~40× faster). Per-test build-path times: 836-2000ms → 2-8ms. Even at 100× CI slowdown, tests stay well under the 5s threshold
  • All 334 tests in packages/codev/src/commands/porch/__tests__/ still pass — verified scope
  • No follow-up "test is genuinely slow" issue needed — the test was never doing meaningful work in the gh call (fallback paths kicked in anyway); the mock just eliminates wasted subprocess startup

Design pattern worth keeping in mind

The general lesson — "if a test is timing out near vitest's default, look for hidden subprocess/network calls before bumping the timeout" — is a good architectural reflex. The fix here didn't add a it('...', { timeout: 15_000 }) band-aid; it removed the real source of slowness. Future flake-investigations in this codebase should follow the same instinct: hypothesize-then-verify the dependency, not workaround the symptom.

Verdict

Approved. Please merge with: gh pr merge 896 --merge --admin (solo-architect branch-protection workaround).

This fix is purely in packages/codev/, not packages/vscode/, so no VS Code CHANGELOG entry needed.


Architect review

@amrmelsayed amrmelsayed merged commit 9c0ee26 into main May 28, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

porch: flaky 5s timeout in next.test.ts "emits BUILD tasks for fresh specify phase"

1 participant