fix(app-cli): give the suite explicit test and hook timeouts - #2244
fix(app-cli): give the suite explicit test and hook timeouts#2244willgriffin wants to merge 1 commit into
Conversation
bridge.test.ts spawns tsx on a fixture through StdioClientTransport and completes an MCP handshake before asserting. The package declared no testTimeout, so that ran against vitest's 5s default. It passed locally and on the affected lane, then timed out three times in the merge queue -- where mode: full actually runs this package -- and ejected an unrelated CI-only PR. Set both budgets explicitly at 30000, the value the rest of the workspace uses; hooks do not inherit testTimeout. Closes #2243
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR makes @happyvertical/smrt-app-cli’s Vitest suite resilient in CI/merge-queue runs by explicitly setting suite-level testTimeout and hookTimeout, avoiding failures from Vitest’s low default timeouts when tests spawn child processes.
Changes:
- Set
testTimeout: 30000for theapp-clitest suite. - Set
hookTimeout: 30000alongsidetestTimeoutto ensure hooks have the same budget. - Add inline rationale documenting why the increased budgets are needed in this package.
| // bridge.test.ts spawns `tsx` on a fixture through StdioClientTransport and | ||
| // completes an MCP handshake before it asserts anything. On vitest's 5s | ||
| // default that passed locally and on the affected lane, then timed out | ||
| // three times in the merge queue — where `mode: full` actually runs this | ||
| // package — and ejected an unrelated PR (#2243). 30000 matches the value | ||
| // used across the rest of the workspace. |
|
Removed from the merge queue — reason: MANUAL Someone removed this entry. Removal also restarts validation for every entry that was queued behind it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31065704626 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
|
Superseded by #2245, which carries this commit cherry-picked verbatim alongside the other three patch-class changes. Consolidated to spend one validation pass instead of four. For #2205 and #2233 specifically it also resolves a real collision — both rewrite Branch preserved. |
{"schema":"hv-agent-run:v1","runtime":"claude","session":"8a94e288-0358-403f-a3a6-8e81a80e6c5d","issue":"2243","head_sha":"82b1abd2a9b597d387262b35feb5d91b36bdedb8","policy_revision":"1.0.0","status":"complete"}Summary
packages/app-cli/vitest.config.tsdeclared notestTimeout, so the suite ran on vitest's5 s default.
src/__tests__/bridge.test.ts > runMcpStdioBridgeis not a 5 s test — it spawnstsxon a fixture throughStdioClientTransportand completes an MCP handshake before itasserts anything.
Impact
Load-sensitive, so it passes on the PR lane and fails in the merge queue, where
mode: fullactually runs this package. A PR sits at CLEAN and is then ejected on queue entry by a
failure in a package it never touched:
That ejected #2242 — a CI-only change of workflow YAML plus two docs files — after it had
already spent a full validation cycle.
Change
Both budgets set explicitly at
30000, the value used by 45 other packages in the workspace.hookTimeoutis set alongsidetestTimeoutper the rule that landed in #2242: hooks do notinherit it.
This is the third instance of one defect class in a day —
mcp-conformance-fixture(
hookTimeoutat the 10 s default, fixed three times over in #2238/#2239/#2240),app-clihere, and
usersin #2220. A sweep for suites that spawn processes on default budgets isworth doing separately.
Validation
Config-only change to test budgets; it relaxes limits and cannot mask a failure, since a
genuinely hung spawn still fails, just later. The merge-queue run exercises the fix directly,
because that is the lane where this package's tests actually execute.
Closes #2243