fix(cli): size the vitest timeout to what the suite actually does - #88
Merged
Conversation
Much of this suite is integration-shaped: tests in check, runtime-check,
init, and onboard spawn the real built CLI with execFile("node",
[binPath, ...]), and several lay out a fixture tree and run `git init`
first. One cold CLI spawn measures ~0.8s by itself, so a test doing four
or five of them sits at 3-4s before any load.
Vitest's default 5s testTimeout left no margin for that. Because vitest
runs test files in parallel workers competing for CPU, whichever tests
landed together tipped over, so a different set failed on each run,
always with "Test timed out in 5000ms" and never an assertion failure.
That reads as a flaky product when it is a timeout sized below the work.
Confirmed to predate any current branch: a clean checkout of main with no
local changes failed 8 tests this way. With the timeout at 20s the suite
passes 427/427 across three consecutive runs.
20s swallows the contention without hiding a genuine hang.
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts Vitest configuration for @taskless/cli so the integration-heavy CLI test suite has realistic timeouts and stops failing due to the default 5s limit when workers contend for CPU.
Changes:
- Adds a
testblock topackages/cli/vite.config.tsto settestTimeoutandhookTimeoutto 20s. - Documents the rationale and observed behavior behind the timeout increase (integration-style tests spawning the built CLI, fixture setup,
git init, parallelism effects).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CLI test suite fails a rotating handful of tests on every run, always with
Test timed out in 5000msand never an assertion failure. It is not flaky code. The timeout is sized below the work.Much of this suite is integration-shaped rather than unit-shaped. Tests in
check,runtime-check,init, andonboardspawn the real built CLI throughexecFile("node", [binPath, ...]), and several lay out a fixture tree and rungit initfirst. One cold CLI spawn measures ~0.8s on its own, so a test doing four or five of them sits at 3-4s before any load at all. Vitest runs test files in parallel workers that compete for CPU, so whichever tests happen to land together are the ones that tip over, which is why the failing set changes run to run.This raises
testTimeoutandhookTimeoutto 20s. That swallows the contention without hiding a genuine hang.Evidence it predates every open branch
Checked out
origin/mainatd7abf6awith no local changes and no merge, ran the suite, and got 8 failures, all timeouts. So this is the suite's own sizing rather than anything a current branch introduced.With the change,
packages/clipasses 427/427 across three consecutive runs.Note for reviewers
packages/clihas novitest.config.ts; vitest readsvite.config.ts, which had notestblock, so the 5s default applied. The new block is test-only configuration and does not affect the published bundle.Worth knowing separately: running
vitestwithout building first fails ~119 tests, because the integration tests spawndist/index.js.turbohandles this viatest.dependsOn: ["build"], sopnpm testat the root is fine; a barepnpm --filter @taskless/cli testin a fresh checkout is not.