ReviewOS

also looking at this

stacks/bun-query-builder

test(cli): run the CLI in one process instead of two

#1192
Merged glennmichael123 wants to merge test/cli-single-process-spawn into main
1 file +39 -27
8 days ago

The flakiest file in the suite, and why

Every one of the 64 tests in cli.test.ts spawned two processes. runCli wrote a runner script to a temp dir, spawned Bun to execute it, had that process Bun.spawnSync the CLI and write its result to a JSON file, then read the file back and removed the directory — all under bun:test's default 5000ms budget, with no explicit timeout anywhere in the file.

Under load the budget killed the outer process mid-flight, so the failure read:

CLI test runner exited null

A message about the harness, naming neither the command that was running nor the timeout — and bin/qbx --version was never the thing that was slow. A bare bun test run earlier today lost 13 tests this way.

The indirection bought nothing

It arrived in 8b227d0, the commit that also switched CLI_PATH from bin/cli.ts to the compiled bin/qbx, with a one-line message and no explanatory comment. That reads as scaffolding for the switch rather than a workaround: bin/qbx is a native executable, and node's spawnSync runs it directly from inside bun:test without complaint.

Measured

Same 64 tests, all passing either way:

runsspread
before18.66s, 35.84s, 30.89s17s
after10.05s, 10.99s, 11.53s, 9.55s2s

The spread matters more than the mean. The old file wasn't so much slow as unpredictable, and unpredictable against a fixed budget is what produces a flake. Raising the timeout would have hidden that; removing a process layer fixes the cause.

Whole-suite bun run test drops 70.62s → 29.33s, since this file dominated it.

Two failure modes now reported instead of disguised

  • Missing binary says so and names the fix — bun run test builds first, a bare bun test does not. Verified by pointing it at a nonexistent path: CLI did not run (/nonexistent/qbx): ENOENT ... Run \bun run build`.` Previously this surfaced as an opaque runner exit.
  • status is left null when a signal killed the process rather than coerced, because callers assert [0, 1] contains the code and a coerced null would read as a pass.

Also confirmed a non-zero CLI exit still arrives as 1 rather than throwing, which the expect([0, 1]) assertions rely on.

Verification

Canonical bun run test: 2522 pass, 0 fail, 219 files. bun run lint 0 errors, none in this file. tsc clean. All imports still used — no import changes needed.

🤖 Generated with Claude Code

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.