Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 6:39 PM PT - Jul 28th, 2026
❌ @robobun, your commit c73f418 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36205That installs a local version of the PR into your bun-36205 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — the describe.concurrent race I flagged earlier is resolved (803d8e2 reverts to serial describe and adds a comment explaining why).
What was reviewed:
- Confirmed
--workspaces/--filterroute throughfilter_run.rswhich flushes all output (including the "Exited with code" line) to stdout in the non-TTY path, so the newexpect(stderr).toBe("")is safe. - Verified the
test.eachcollapse preserves both original scenarios (only the--filtervariant's distinct stdout marker string was dropped, which is fine since the assertion is a substring match). - No assertions weakened;
await usingon the spawned procs andcwd: String(dir)match harness conventions.
Extended reasoning...
Overview
Test-only cleanup of test/regression/issue/26207.test.ts: two near-identical --workspaces/--filter tests are collapsed into a single test.each, all three cases gain expect(stderr).toBe(""), subprocesses use await using, and a comment documents why the file must stay serial. Net -29 lines. No production code touched.
Prior review addressed
My earlier review on this PR flagged describe.concurrent as introducing a debug-build race on the shared /tmp/bun-node-debug directory. Commit 803d8e2 dropped .concurrent, reverting to serial execution, and the PR description now documents the race and why concurrency was abandoned. The added file-level comment ("These cases must stay serial…") captures this for future readers.
Security risks
None. Test-only change with no production code modified.
Level of scrutiny
Low. The main risk with a test refactor like this is silently weakening coverage or introducing flakiness. I checked both:
- Coverage: the
test.eachmatrix produces the same twocmdarrays as the original tests; the shebang test is unchanged apart from the added stderr check. The only behavioral delta is that the--filtercase now asserts"node works"instead of"node works from filter", which is fine — both variants share one fixture andtoContainis a substring match. - New stderr assertion: I traced
bun run --workspaces/--filtertofilter_run::run_scripts_with_filter(src/runtime/cli/mod.rs:1440). With piped stdout,pretty_outputis false andflush_draw_buf()writes exclusively toFile::stdout()(filter_run.rs:570), including the exit-status line — nothing goes to stderr on success. The commented-outexpect(stderr).toBeEmpty()infilter-workspace.test.ts:137gave me pause, but that file's helper covers many more scenarios; for the single-package success path here, stderr is empty. The author also confirmed 9 passing expects locally.
Other factors
The change follows REVIEW.md guidance directly: test.each for matrices, await using for subprocess cleanup, asserting stderr before exitCode, and documenting why a slow file can't use test.concurrent. No existing test is deleted, skipped, or weakened.
All three cases spawn an independent bun run --workspaces/--filter
subprocess (which itself spawns bun-as-node), so there is no shared
state between them. Wrap in describe.concurrent so they overlap
instead of running serially.
Also collapse the two near-identical --workspaces/--filter cases
into a test.each and add an expect(stderr).toBe('') to each case so
unexpected error output fails the test.
In debug builds create_fake_temporary_node_executable wipes the shared bun-node-debug temp dir before recreating it, so concurrent runs can race on the delete/recreate sequence. Keep the test.each collapse, stderr assertions, and await using; leave the cases serial with a comment so a future batch-concurrency pass does not reintroduce the hazard.
803d8e2 to
f5fc9d8
Compare
There was a problem hiding this comment.
LGTM — the describe.concurrent race is reverted to serial describe with a comment naming the shared bun-node-debug dir, so my earlier concern is resolved.
What was reviewed:
- Confirmed the three cases now run serially under plain
describe;test.eachpreserves both--workspacesand--filter '*'variants with identical fixtures/env. - New
expect(stderr).toBe("")— checkedfilter-workspace.test.tsfor prior-art on stderr from the workspace runner; the commented-out assertion there was never enabled (present since file creation), and the author verified 9 passing expects locally underbunEnv(non-TTY, NO_COLOR). await usingontempDir/Bun.spawnandcwd: String(dir)match harness conventions; no test deleted or weakened.
Extended reasoning...
Overview
Test-only refactor of test/regression/issue/26207.test.ts (net −29 lines). Collapses two byte-identical --workspaces / --filter cases into one test.each, adds expect(stderr).toBe("") to all three cases, switches to await using for tempDir and Bun.spawn, and wraps everything in a serial describe with a comment explaining why concurrency is unsafe here (create_fake_temporary_node_executable wipes the shared bun-node-debug dir in debug builds). No production code touched.
Prior review resolution
An earlier revision used describe.concurrent, which I flagged as racing on the fixed /tmp/bun-node-debug directory since every case forces the fake-node fallback. The author reverted to serial describe in f5fc9d8 and replaced the misleading "safe to run in parallel" comment with one that accurately names the shared resource and the debug-build wipe. The current diff matches that fix; the inline thread is resolved.
Security risks
None. Test file only; spawns the local debug binary with bunEnv, no network, no external hosts, no credential handling.
Level of scrutiny
Low. This is a mechanical test dedup following repo conventions from CLAUDE.md/REVIEW.md (test.each for matrices, await using for resource cleanup, assert stderr/stdout before exitCode). The same three scenarios are covered with strictly stronger assertions. The one substantive addition — asserting stderr is empty — was verified by the author under bun bd test (9 passing expects), and bunEnv + piped (non-TTY) stderr means the workspace runner's progress UI is suppressed. I checked test/cli/run/filter-workspace.test.ts:137 where a similar assertion is commented out; git log -S shows it was authored that way from the start rather than disabled after flaking, so it's not evidence against this assertion.
Other factors
cwd: String(dir)vs the previouscwd: dir— both work (tempDir's return stringifies to the path);String(dir)matches the CLAUDE.md example.- The
test.eachlabel/flags tuple correctly spreads["--filter", "*"]into the cmd array, matching the original hand-written invocation. test.skipIf(isWindows)on the shebang case is preserved unchanged.- No outstanding human review comments; CodeRabbit was rate-limited and left no findings.
There was a problem hiding this comment.
LGTM — the describe.concurrent concern from my earlier review is addressed (now plain serial describe with a comment naming the shared bun-node-debug dir), and what remains is a straightforward test dedupe with strictly stronger assertions.
What was reviewed
- Confirmed the three cases still run serially;
test.eachpreserves both--workspacesand--filter '*'coverage. tempDirusesmkdtempSync, so the shared26207-node-fallbackprefix across the twotest.eachcases still yields unique dirs.- New
expect(stderr).toBe("")+await using procfollow harness conventions; stdout/exitCode ordering matches CLAUDE.md guidance.
Extended reasoning...
Overview
Test-only cleanup of test/regression/issue/26207.test.ts: the two near-identical --workspaces / --filter cases are collapsed into a single test.each over the flag list, each case gains an expect(stderr).toBe("") assertion (stderr was previously piped but never read), spawned subprocesses now use await using, and a comment documents why the file must stay serial. No production code is touched; net -29 lines with the same three scenarios covered.
Prior review resolution
My earlier review flagged that an intermediate revision used describe.concurrent, which would race on the shared /tmp/bun-node-debug fake-node dir in debug builds. The author reverted to plain describe in f5fc9d8 and replaced the inline comment with one that names create_fake_temporary_node_executable and the shared temp dir as the reason the cases must stay serial. The current diff reflects that fix — the describe is serial and the comment is accurate. The inline thread is marked resolved.
Security risks
None. This is a regression test file; no runtime, auth, crypto, or network-facing code is modified.
Level of scrutiny
Low. Test-only, mechanical refactor. I checked that (a) test.each covers both original flag variants with the same fixture and env, (b) tempDir uniquifies via mkdtempSync so the shared prefix is harmless, (c) cwd: String(dir) matches the harness pattern for the DisposableString wrapper, and (d) the shebang case is unchanged apart from the added stderr assertion and await using. The only new behavioral surface is expect(stderr).toBe(""), which the author verified passes locally under bun bd test; if a platform emits stderr noise CI will surface it immediately and the fix is trivial.
Other factors
Assertions are strictly stronger (9 vs 6 expect calls), no test is deleted or skipped, ordering follows the stdout-before-exitCode convention, and pipes are drained concurrently via Promise.all. Nothing outstanding from other reviewers.
|
CI on this branch is red only on tests this diff does not touch: the windows x64 verify-baseline lane failed to provision an agent, and the remaining failures ( |
|
Closing this PR because it has been inactive for more than 90 days. |
What does this PR do?
Cleanup of
test/regression/issue/26207.test.ts:--workspaces/--filtercases into a singletest.eachover the flag variantexpect(stderr).toBe("")to each case so unexpected error output fails the test (stderr was previously piped but never checked)await usingfor the spawned subprocessesNo tests are deleted or skipped; the same three scenarios are covered with stronger assertions. Net -29 lines.
No speedup. This started as an attempt to make the file faster on ASAN (18.4s in
expected-durations.json) viadescribe.concurrent, but every case forces the fake-node fallback path and in debug buildscreate_fake_temporary_node_executableunconditionally wipes the sharedbun-node-debugtemp dir (src/install/lib.rs:578-583on POSIX,:702-720on Windows) before recreating it, so the three concurrent subprocesses can race on that delete/recreate sequence. Running the three cases concurrently 240 times locally (8-way, 30 rounds) did not trigger the race, buttest/cli/install/bun-run.test.tsalready documents the same hazard in its Windows-side comment and I don't want to add a second dependency on it. Making the POSIX path race-tolerant (its own inline comment already notes thedelete_treeis redundant given the EEXIST handling below it) would unblock concurrency here but is out of scope for a test-only change.How did you verify your code works?
bun bd test test/regression/issue/26207.test.tspasses with 9expect()calls (up from 6). Wall time is unchanged vsmainsince the cases remain serial.no test proof · iteration 1 · Platform-specific test-only change; deferring to CI.