Repository navigation
Conversation
…ct output The file ran 88 tests one at a time through spawnSync, and its fixture scripts waited on fixed timers (three 100 ms sleeps per script, 300 ms keep-alives). On the ASAN lane that took 13 s. Every test now spawns with Bun.spawn inside describe.concurrent. Scripts whose content does not matter are shell builtins (echo, cat, seq) instead of a bun child. The "run in parallel" and cycle tests synchronize the two scripts on marker files instead of sleeping. The dependency-order tests give the dependency a runtime start and the dependent a plain cat, so a run that started both together reads a missing file. Assertions compare the sorted stdout lines and the full stderr to exact values. The --elide-lines tests read the final renderer frame and check which lines were kept. The error cases pin the exact messages. The cycle-of-three test names its third package dep2, so the cycle is a real three-cycle. The "elides by default" case no longer passes --elide-lines. The four tests that put bun behind the `node` shim stay serial: every bun process shares the shim directory and a debug build deletes it before it writes the shim again.
|
Status: ready for review. Reproduced the slow run locally with the debug build: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe workspace filter test suite now uses asynchronous ChangesWorkspace filter tests
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This test-only change improves execution speed and assertion precision without introducing a supported production risk; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, verification steps, test coverage, and known concurrency constraint. It does not use the exact template headings, but it provides the required information in equivalent sections. Comment |
|
Updated 4:05 AM PT - Aug 31st, 2026
✅ @robobun, your commit d123ea4ad4d3418d95d8e02e03e0e08d5b6c6a60 passed in 🧪 To try this PR locally: bunx bun-pr 41033That installs a local version of the PR into your bun-41033 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites every test in the file — swapping regex substring checks for exact-output assertions and replacing timing-based concurrency checks with new mechanisms — a human pass to confirm no coverage was silently narrowed and to coordinate with the nine open PRs it conflicts with would still be worthwhile.
What was reviewed:
- The
run()/sorted()/finished()helpers and every call site — the exact sorted-line comparisons are strictly tighter than the oldtoMatch/antipatternpairs, and stderr is now asserted everywhere it was previously ignored. - The rendezvous script (bounded 10s poll, no fixed sleep) and the
write.js-vs-catdependency-order asymmetry — both replace 100ms/300ms sleeps with observable conditions. lastFrame()parsing of the\x1b[?2026hsynchronized-update marker and the Windows non-TTY branch in the elide tests;seq/cat/echoare Bun-shell builtins so the Windows path holds.- The length-3 cycle fix (
dep2was previously a duplicatedep1), the[run] bun = truetests staying serial per the shim-directory race note, and resource cleanup (usingon everytempDir, detached-child pids reaped in both output-timing tests).
Extended reasoning...
Overview
This PR is a full rewrite of test/cli/run/filter-workspace.test.ts (1291 → 999 lines, every one of the ~88 tests touched). It replaces spawnSync + toMatch(/regex/) helpers with an async Bun.spawn wrapper and exact sorted-line/stderr assertions, moves almost everything into describe.concurrent, drops the 100ms/300ms sleep-based interleave/order checks in favor of a filesystem rendezvous (bounded poll) and a runtime-start-vs-cat asymmetry, and rewrites the --elide-lines tests to compare the terminal renderer's final synchronized-update frame exactly instead of grepping the byte stream. Fixtures are trimmed (unused index.js/sleep.js removed; seq 1 20 replaces a 20-line bun script). No runtime code is changed.
Security risks
None. The change is test-only, spawns only the local bunExe() against temp-dir fixtures, contacts no network, and touches no auth/crypto/permissions paths.
Level of scrutiny
Moderate-to-high, driven entirely by size and coverage semantics rather than risk. REVIEW.md's "never silently weaken, skip, or delete an existing test" rule applies to every case here since each was replaced rather than edited in place — I traced the old target_pattern/antipattern pairs into the new exact sorted(stdout).toEqual(finished(...)) shapes and found them uniformly tighter (the exact list subsumes both the positive and negative regex checks), but a maintainer should independently spot-check that the pinned error/warning strings, the --parallel renderer format, and the lastFrame output are contracts they're comfortable locking down. The change also lands squarely on the "large" side of the approval guidelines.
Other factors
The PR description states this rewrite conflicts with nine open PRs that add cases to the same file — merging needs human coordination on sequencing. The changed file is not covered by CODEOWNERS. There are no prior reviews or outstanding objections in the timeline. The author reports 23 debug Linux runs and 5 Windows release runs green, plus USE_SYSTEM_BUN=1 passing on both platforms; the harness conventions (bunExe/bunEnv, tempDir with using, Promise.all pipe drain, stdout-before-exitCode ordering, normalizeBunSnapshot) are followed throughout, and the four [run] bun = true shim tests are deliberately kept serial with a comment naming the debug-build shim-directory race (#37384). The bug hunt ran to a dry streak with no findings and no ruled-out candidates.
Problem
test/cli/run/filter-workspace.test.tstook 13 s on the x64-asan lane (build 108747). 88 tests ran one at a time throughspawnSync, and the fixture scripts slept 100 ms and 300 ms on fixed timers.toMatch(/scripta/)on stdout. Nothing checked stderr.Fix
Bun.spawninsidedescribe.concurrent. Scripts whose content does not matter are shell builtins (echo,cat,seq). The "run in parallel" and cycle tests synchronize on marker files. The dependency-order tests pair a bun child with a plaincat, so a run that starts both together reads a missing file.--elide-linestests check the final renderer frame, whose lines are1to20, so elision of the wrong end fails. Error cases pin the exact messages.Background
bun run --filterruns one script per matched package, in parallel unless one depends on another. A pipe gets<package> <script>: <line>per line, then<package> <script>: Exited with code N. Packages interleave, so the tests sort the lines.[run] bun = trueputs anodeshim on PATH. All bun processes share the shim directory, and a debug build deletes it first (install: stop debug builds from wiping the shared node shim dir on every start #37384 fixes that). The four shim tests stay serial.Notes
Assertion changes, test by test:
--filterrun:sorted(stdout)equals the exact list of<package> <script>: <line>andExited with code Nlines, and stderr equals the exact text ("", or themalformed2warning for the shared fixture). Before:toMatchon a few words, stderr unchecked, the absence checks (antipattern) are covered by the exact list../packages/*,./**,./packages/malformed1): the lines of the package without a name appear with an empty package name (present: malformed1).notpresent,x): stderr is exactly themalformed2warning pluserror: No workspace packages matched the filter "*". Before: a regex on part of it.warn: No workspace packages matched the filter "typo".--elide-lines(default, 15, 0) and the two bunfig elide tests: the last synchronized-update frame, with ANSI escapes stripped and the duration replaced, equals the exact frame text, including[N lines elided]and the kept linesN+1to20. Before: "at least 20log_linesomewhere in the byte stream", which test(filter-workspace): assert --elide-lines on the final redraw frame #35620 found to be chunk-timing dependent. The default case no longer passes--elide-lines 10, so it tests the default. On Windows the plain output is compared instead.dep0 script $ exit 0and└─ Done in <time>.<me> saw <other>only when both were alive at the same time (marker files, 10 s deadline). Before:x...y...xinterleaving of 100 ms sleeps.echo > out.txt,cat out.txt && echo > post.txt,cat post.txt).cat ../<dep>/out.txt, the dependency writes it from a bun script. Exact output, no 100 ms sleep.dep2(it wasdep1, a typo, so the cycle had length 2 and two packages shared a name).Exited with code 0andExited with code 23.cyctwice.test.eachover twelve(patterns, expected packages)rows, exact output, stderr"".--parallel: exact stdout (api:present | out-api) and exact stderr status lines with the duration replaced....: stderr is exactlyerror: --filter "..." is missing a workspace name or path.pkg-bg go: Signaled with code SIGINT, stderr"", exit 130. Before:expect.any(String)."".[run] bun = true(three forms) and--bun: exact lines. Malformed bunfig: inline snapshot of the full error.Fixture changes: the unused
index.jsfiles and thesleep.jsscripts are gone from the shared workspace. The--elide-linesfixtures runseq 1 20instead of a bun script with twentyconsole.logcalls. The>8 characterscase writes a fixed string instead of a timestamp.Flake found while testing: with four copies of the file running at once on the debug build, one
[run] bun = truerun printedBun is undefined.create_fake_temporary_node_executableinsrc/install/lib.rsdeletes the shared/tmp/bun-node-debugdirectory before it writes the shim again (debug builds only), so a concurrent run can remove thenodelink between another run's setup and its script's PATH lookup. #37384 removes that delete. Until then the four shim tests in this file run one at a time. No failure in 23 single-copy debug runs, and none in 8 four-copy runs of the final file (the one failure was in 12 four-copy runs of the version with concurrent shim tests).Timings per test with the debug build on this machine: the
echoruns take 130 to 230 ms each, the tests with a bun child 380 to 600 ms. The release bun runs the whole file in 0.13 s. The per-test concurrency under ASAN is 5 (max_concurrencydefault insrc/options_types/context.rs), so the child count matters as much as the parallelism.The rewrite touches every test in the file, so the open PRs that add cases to it (#35620, #35573, #36656, #38101, #40546, #39650, #29941, #35580, #37131) will need their cases re-added in the new shape. #35620's final-frame approach is what the
--elide-linestests here do.Also run:
USE_SYSTEM_BUN=1 bun test test/cli/run/filter-workspace.test.ts(release, 87 pass), the file 5 times withUSE_SYSTEM_BUN=1on windows-x64 (85 pass, 4 skipped as before; 3.5 s before, 0.62 s after), andtsc --noEmiton the file with the test tsconfig (clean).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/filter-workspace.test.ts