test: preserve Windows CI timeouts in isolated tests - #608
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed b1759c180777e0c6490d0be43e6196d379b2bfb8. This is a necessary test-infrastructure consistency fix; no blocking correctness issue found.
Necessity
The Windows runner already grants 120 seconds per test, but isolated tests are launched by another Bun process with a separate hard-coded 30-second budget. Increasing the outer runner's budget alone cannot fix that mismatch. Passing the existing value into the child repairs the actual boundary; it does not merely extend an unrelated timeout or disable a test.
Current main (227fc32ac47c9ac5d78f6dd246d8899673a67858) still has the hard-coded child timeout, so this change remains relevant. The default outside the Windows runner stays 30 seconds, and the outer CI shard deadline remains the final bound.
Implementation and simplification
The test-only environment handoff and one fallback in the child helper are enough. Keep this out of production configuration and avoid adding a general timeout-options abstraction, retries, or platform-specific skips. The value is owned by the test runner, so additional production-style validation would not solve a demonstrated problem here.
The regression is usefully behavioral: it sets a deliberately short child budget and observes the isolated test time out. The only non-blocking fragility is the exact Bun diagnostic text (this test timed out after 100ms). It is acceptable with a pinned Bun version, but a future runner upgrade may change wording without breaking propagation. If this becomes a maintenance problem, assert timeout classification/budget rather than weakening the test to any nonzero exit.
Verification
Ran bun test --timeout 30000 tests-ts/skeleton.test.ts at this head: 9 passed, 0 failed, Bun 1.3.14/Linux using locally available dependencies. This included the nested subprocess regression. I did not rerun the Windows shards or independently certify the CI runs listed in the PR description.
Summary
Fix the Windows CI timeout mismatch exposed by the failing main-branch run. Windows shards allow 120 seconds per test, but isolated child tests still used a hard-coded 30-second deadline.
Changes
Testing
bun test --timeout 30000 --seed 12345 tests-ts/skeleton.test.ts— 9 passed.pnpm run types— passed.pnpm run format— passed.node scripts/run-windows-ci-tests.mjson Windows, with machine-wide policy tests disabled — 1,517 passed, 58 skipped, and two native permission/plugin-cache failures. Both failed cases passed when rerun separately. The affectedapi.test.tsshard passed all 117 tests.Risk and rollout
Test infrastructure only; no shipped CLI, SDK, credential-handling, or public API changes. The Windows 120-second test budget and ten-minute shard limit are unchanged.
Public disclosure review