runner: --parallel-batch runs tests via bun test --parallel - #29654
Jarred-Sumner wants to merge 7 commits into
Conversation
`scripts/runner.node.mjs --parallel-batch` partitions discovered tests into a single `bun test --parallel` invocation (fast path) and a per-file sequential tail. The batch writes a merged JUnit; per-file `<testsuite failures=...>` is the pass/fail signal. A file counts as passed only when it appears with `failures="0"` and no suite for it has `failures>0"` — crashed workers and files with unhandled errors get no `file=` suite, so they fall through to the existing per-file `runTest()` retry/annotation path along with the denylist. Denylist: - `test/no-parallel.txt` (explicit; `# manual` section survives regeneration) - path heuristics in `isParallelSafe()`: `napi/`, `v8/`, `ffi/`, `webview/`, the node-test/cluster trees, and basenames matching `leak|stress|memory|heap|gc|rss` - on ASAN, anything in `no-validate-exceptions.txt` / `no-validate-leaksan.txt` (those need per-file env) Regenerate with `bun run regenerate-no-parallel` (`scripts/generate-no-parallel.ts` greps tests for `expectMaxObjectTypeCount`, `Bun.gc(`, `gcTick`, `heapStats`, `generateHeapSnapshot`, `process.memoryUsage`, `.rss`, `heapUsed`, `Bun.WebView`, `withoutAggressiveGC`, `BUN_JSC_forceRAMSize`). Argv auto-chunks above ~30 KB on Windows (CreateProcessW limit) and ~1.5 MB elsewhere; on Linux/macOS shards it stays a true single batch. Also drops the old `--parallel` (pLimit-based one-process-per-file pool) and its `p-limit.mjs`/`yocto-queue.mjs` vendored deps; the per-file loop is now a plain sequential `for…of`.
|
Updated 11:59 PM PT - Apr 23rd, 2026
❌ @Jarred-Sumner, your commit f2181cc has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29654That installs a local version of the PR into your bun-29654 --bun |
isParallelSafe() now only does the .txt lookup plus the structural check (node-test/cluster trees run via `bun run`, not `bun test`). The napi/v8/ffi/webview path matches and the leak/stress/memory/heap/gc/rss basename match move into scripts/generate-no-parallel.ts so the .txt is the single source of truth — 214 entries, one place to look.
WalkthroughAdds an auto-generated test denylist and its generator script and docs, removes two queue/concurrency utilities, refactors the test runner to run parallel-safe tests in a batch phase (using Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate-no-parallel.ts`:
- Around line 40-45: isPathExcludedByRunner currently misses some path
exclusions the runner applies; update the function (isPathExcludedByRunner) to
mirror the runner's full path exclusions by adding checks for
"js/bun/test/parallel/" and for "js/node/cluster/test-*.ts" (and any other
exclusions present in scripts/runner.node.mjs) — i.e., extend the existing regex
checks to include /js\/bun\/test\/parallel\// and a pattern matching
/js\/node\/cluster\/test-.*\.ts/ (case-insensitive consistent with existing
rules) so files excluded by the runner aren’t included by generate-no-parallel.
In `@scripts/runner.node.mjs`:
- Around line 1401-1407: The batch-path env setup currently only enables
exception validation (BUN_JSC_validateExceptionChecks,
BUN_JSC_dumpSimulatedThrows) when isAsan || !isCI, which drops leak-sanitizer
semantics compared to the sequential path; update the batch env block that
builds env so that when isAsan || !isCI you also set BUN_DESTRUCT_VM_ON_EXIT and
propagate the same ASAN_OPTIONS and LSAN_OPTIONS used by the sequential path
(unless the test is listed in test/no-validate-leaksan.txt), ensuring batched
tests run under the same ASAN/LSAN/leak-destruction settings as the sequential
tail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1faf60b-1902-4903-845a-72701a3be012
📒 Files selected for processing (7)
.claude/commands/regenerate-no-parallel.mdpackage.jsonscripts/generate-no-parallel.tsscripts/p-limit.mjsscripts/runner.node.mjsscripts/yocto-queue.mjstest/no-parallel.txt
💤 Files with no reviewable changes (2)
- scripts/yocto-queue.mjs
- scripts/p-limit.mjs
| const env = { GITHUB_ACTIONS: "true" }; | ||
| // Per-file env denylists can't apply to a shared batch process; route | ||
| // those files to the sequential tail instead so they get correct env. | ||
| if (isAsan || !isCI) { | ||
| env.BUN_JSC_validateExceptionChecks = "1"; | ||
| env.BUN_JSC_dumpSimulatedThrows = "1"; | ||
| } |
There was a problem hiding this comment.
The batch path currently drops LSAN coverage on ASAN/local runs.
The sequential path still enables BUN_DESTRUCT_VM_ON_EXIT plus ASAN_OPTIONS/LSAN_OPTIONS for tests that are not in test/no-validate-leaksan.txt, but the shared batch env only turns on exception validation. That means batched tests stop running under leak-sanitizer semantics entirely, so --parallel-batch weakens sanitizer coverage instead of preserving it.
Suggested fix
const env = { GITHUB_ACTIONS: "true" };
// Per-file env denylists can't apply to a shared batch process; route
// those files to the sequential tail instead so they get correct env.
if (isAsan || !isCI) {
env.BUN_JSC_validateExceptionChecks = "1";
env.BUN_JSC_dumpSimulatedThrows = "1";
+ env.BUN_DESTRUCT_VM_ON_EXIT = "1";
+ env.ASAN_OPTIONS = "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1:abort_on_error=1";
+ env.LSAN_OPTIONS = `malloc_context_size=100:print_suppressions=0:suppressions=${process.cwd()}/test/leaksan.supp`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/runner.node.mjs` around lines 1401 - 1407, The batch-path env setup
currently only enables exception validation (BUN_JSC_validateExceptionChecks,
BUN_JSC_dumpSimulatedThrows) when isAsan || !isCI, which drops leak-sanitizer
semantics compared to the sequential path; update the batch env block that
builds env so that when isAsan || !isCI you also set BUN_DESTRUCT_VM_ON_EXIT and
propagate the same ASAN_OPTIONS and LSAN_OPTIONS used by the sequential path
(unless the test is listed in test/no-validate-leaksan.txt), ensuring batched
tests run under the same ASAN/LSAN/leak-destruction settings as the sequential
tail.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
LSAN reports at process exit; --parallel workers run many files per process under --isolate, so a batch leak report can't be attributed to a file. Gating the batch off for ASAN keeps today's per-file LSAN coverage exactly, and drops the half-implemented needsPerFileEnv routing (skipsForLeaksan term was dead — the batch never enabled leaksan). Non-ASAN shards still get the batch.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/runner.node.mjs (1)
1399-1405:⚠️ Potential issue | 🟠 MajorPropagate leak-sanitizer env into the batch fast path.
The per-file path still enables
BUN_DESTRUCT_VM_ON_EXIT,ASAN_OPTIONS, andLSAN_OPTIONS, but the shared batch env only turns on exception validation. That weakens leak coverage for every ASAN/local test that stays in the fast path.♻️ Suggested fix
if (isAsan || !isCI) { env.BUN_JSC_validateExceptionChecks = "1"; env.BUN_JSC_dumpSimulatedThrows = "1"; + env.BUN_DESTRUCT_VM_ON_EXIT = "1"; + env.ASAN_OPTIONS = "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1:abort_on_error=1"; + // prettier-ignore + env.LSAN_OPTIONS = `malloc_context_size=100:print_suppressions=0:suppressions=${process.cwd()}/test/leaksan.supp`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/runner.node.mjs` around lines 1399 - 1405, The batch-fast-path env currently only enables BUN_JSC_validateExceptionChecks and BUN_JSC_dumpSimulatedThrows when isAsan || !isCI, which omits the leak-sanitizer settings; modify the same conditional that sets BUN_JSC_validateExceptionChecks/BUN_JSC_dumpSimulatedThrows to also set BUN_DESTRUCT_VM_ON_EXIT, ASAN_OPTIONS, and LSAN_OPTIONS on the env object so the shared batch process receives the same leak-sanitizer configuration as the per-file path (refer to the env variable and the isAsan/isCI condition and the existing BUN_JSC_* keys to locate where to add these keys).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate-no-parallel.ts`:
- Around line 41-44: The generator's isNodeTestTree function currently excludes
both "parallel" and "sequential" test folders while the runner's isNodeTest does
not; update isNodeTestTree's regex to match the runner's logic (i.e., remove the
"sequential" alternative) so both functions use the same exclusion pattern.
Locate the isNodeTestTree function and replace the regex
/(^|\/)js\/(node|bun)\/test\/(parallel|sequential)\//.test(posix) with the
runner-equivalent pattern that only checks for the "parallel" folder (keeping
the same surrounding anchors), ensuring parity between isNodeTestTree and
isNodeTest.
In `@scripts/runner.node.mjs`:
- Around line 1438-1462: The code currently adds every <testsuite file=...>
(including nested describe suites) to seenSuffixes/failedSuffixes, allowing
nested suites to make a file appear "seen" or "passed" even if the top-level
file suite wasn't emitted; update the parsing loop that uses suiteRe so it only
treats file-level suites (where the captured name equals the captured file,
i.e., m[1] === m[2]) as authoritative before calling seenSuffixes.add(...) or
failedSuffixes.add(...), leaving matchesSuffix, testPaths and passed logic
unchanged so retries still occur when the top-level file suite is missing.
- Around line 1384-1425: The batch JUnit XML is currently written to a temporary
mkdtempSync(junitDir) and never moved into the user-provided
cliOptions["junit-temp-dir"], so batch reports are missed; change the block that
handles junitFile (inside the chunks loop where junitFile, lastJunit,
parsePassedFilesFromJunit, and addToJunitUploadQueue are used) to persist the
file into cliOptions["junit-temp-dir"] when that option is set: create the
target dir if needed, copy or move junitFile into that directory (update
lastJunit to the new path or keep both as needed), then call
parsePassedFilesFromJunit against the persisted path and still call
addToJunitUploadQueue only when cliOptions.junit && isBuildkite &&
cliOptions["junit-upload"] are true; this ensures batch JUnit reports end up in
the configured junit-temp-dir even when junit-upload is false.
---
Duplicate comments:
In `@scripts/runner.node.mjs`:
- Around line 1399-1405: The batch-fast-path env currently only enables
BUN_JSC_validateExceptionChecks and BUN_JSC_dumpSimulatedThrows when isAsan ||
!isCI, which omits the leak-sanitizer settings; modify the same conditional that
sets BUN_JSC_validateExceptionChecks/BUN_JSC_dumpSimulatedThrows to also set
BUN_DESTRUCT_VM_ON_EXIT, ASAN_OPTIONS, and LSAN_OPTIONS on the env object so the
shared batch process receives the same leak-sanitizer configuration as the
per-file path (refer to the env variable and the isAsan/isCI condition and the
existing BUN_JSC_* keys to locate where to add these keys).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6c553f0-d840-4892-9137-41ac1bd480c6
📒 Files selected for processing (4)
.claude/commands/regenerate-no-parallel.mdscripts/generate-no-parallel.tsscripts/runner.node.mjstest/no-parallel.txt
| // Runs via `bun run`, not `bun test` — structurally excluded by the runner. | ||
| function isNodeTestTree(posix: string): boolean { | ||
| return /(^|\/)js\/(node|bun)\/test\/(parallel|sequential)\//.test(posix); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- files under test/js/bun/test/sequential ---"
fd -t f . test/js/bun/test/sequential || true
echo
echo "--- generator exclusion ---"
rg -n -C2 'function isNodeTestTree|js/node/test|js/bun/test' scripts/generate-no-parallel.ts
echo
echo "--- runner exclusion ---"
rg -n -C2 'function isNodeTest|js/node/test|js/bun/test' scripts/runner.node.mjsRepository: oven-sh/bun
Length of output: 2081
Align generator and runner exclusion logic for js/bun/test/sequential/.
The generator's isNodeTestTree() regex includes js/bun/test/sequential/ in its exclusion pattern (line 43), but the runner's isNodeTest() function (at scripts/runner.node.mjs:1745–1759) does not. Although test/js/bun/test/sequential/ does not currently exist, if it is ever created, tests there would bypass test/no-parallel.txt regeneration while the runner could still attempt to batch them—creating a silent inconsistency. Align the exclusion patterns so both use the same logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-no-parallel.ts` around lines 41 - 44, The generator's
isNodeTestTree function currently excludes both "parallel" and "sequential" test
folders while the runner's isNodeTest does not; update isNodeTestTree's regex to
match the runner's logic (i.e., remove the "sequential" alternative) so both
functions use the same exclusion pattern. Locate the isNodeTestTree function and
replace the regex
/(^|\/)js\/(node|bun)\/test\/(parallel|sequential)\//.test(posix) with the
runner-equivalent pattern that only checks for the "parallel" folder (keeping
the same surrounding anchors), ensuring parity between isNodeTestTree and
isNodeTest.
| const junitDir = mkdtempSync(join(tmpdir(), "bun-parallel-batch-")); | ||
| let lastJunit = null; | ||
| const start = Date.now(); | ||
|
|
||
| for (const [i, chunk] of chunks.entries()) { | ||
| const junitFile = join(junitDir, `batch-${i}.xml`); | ||
| const args = [ | ||
| "test", | ||
| "--parallel", | ||
| `--timeout=${perTestTimeout}`, | ||
| "--reporter=junit", | ||
| `--reporter-outfile=${junitFile}`, | ||
| ...chunk, | ||
| ]; | ||
|
|
||
| const env = { GITHUB_ACTIONS: "true" }; | ||
| // Per-file env denylists can't apply to a shared batch process; route | ||
| // those files to the sequential tail instead so they get correct env. | ||
| if (isAsan || !isCI) { | ||
| env.BUN_JSC_validateExceptionChecks = "1"; | ||
| env.BUN_JSC_dumpSimulatedThrows = "1"; | ||
| } | ||
|
|
||
| !isQuiet && | ||
| console.log( | ||
| `${getAnsi("gray")}[batch ${i + 1}/${chunks.length}]${getAnsi("reset")} bun test --parallel (${chunk.length} files)`, | ||
| ); | ||
|
|
||
| await spawnBun(execPath, { | ||
| args, | ||
| cwd, | ||
| timeout: batchTimeout, | ||
| env, | ||
| stdout: chunk => process.stdout.write(chunk), | ||
| stderr: chunk => process.stderr.write(chunk), | ||
| }); | ||
|
|
||
| if (existsSync(junitFile)) { | ||
| lastJunit = junitFile; | ||
| for (const p of parsePassedFilesFromJunit(junitFile, testPaths)) passed.add(p); | ||
| if (cliOptions.junit && isBuildkite && cliOptions["junit-upload"]) addToJunitUploadQueue(junitFile); | ||
| } |
There was a problem hiding this comment.
Persist batch JUnit reports in --junit-temp-dir.
The merged XML is always written to a private mkdtemp() directory. Local --junit runs, and CI runs with --junit-upload=false, never move that file into cliOptions["junit-temp-dir"], so the final report collection misses the entire batch.
🧾 Suggested fix
- const junitDir = mkdtempSync(join(tmpdir(), "bun-parallel-batch-"));
+ const junitDir = cliOptions.junit
+ ? cliOptions["junit-temp-dir"]
+ : mkdtempSync(join(tmpdir(), "bun-parallel-batch-"));
+ mkdirSync(junitDir, { recursive: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/runner.node.mjs` around lines 1384 - 1425, The batch JUnit XML is
currently written to a temporary mkdtempSync(junitDir) and never moved into the
user-provided cliOptions["junit-temp-dir"], so batch reports are missed; change
the block that handles junitFile (inside the chunks loop where junitFile,
lastJunit, parsePassedFilesFromJunit, and addToJunitUploadQueue are used) to
persist the file into cliOptions["junit-temp-dir"] when that option is set:
create the target dir if needed, copy or move junitFile into that directory
(update lastJunit to the new path or keep both as needed), then call
parsePassedFilesFromJunit against the persisted path and still call
addToJunitUploadQueue only when cliOptions.junit && isBuildkite &&
cliOptions["junit-upload"] are true; this ensures batch JUnit reports end up in
the configured junit-temp-dir even when junit-upload is false.
- parsePassedFilesFromJunit only trusts file-level suites (name === file, the is_file_suite marker). Nested describe suites also carry file=; a fragment that crashed mid-file could otherwise mark the file "seen" via an inner suite without the file-level failures= ever landing. - When --junit is on, write batch XML into cliOptions["junit-temp-dir"] so the end-of-run collection finds it; otherwise keep the throwaway mkdtemp.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/runner.node.mjs`:
- Around line 1383-1414: The batch-level timeout (batchTimeout) is being applied
to each chunk via spawnBun, which multiplies the intended wall-clock limit by
the number of chunks; change spawn behavior to enforce the batch timeout across
the whole for-loop by computing remainingBatchTimeout = batchTimeout -
(Date.now() - start) before each spawnBun call (using start already declared)
and pass that remaining time (or bail/throw if <= 0) into spawnBun.timeout
instead of batchTimeout; keep perTestTimeout untouched in the args array and
continue to use spawnBun and chunks/args as-is, but replace the constant
batchTimeout passed to spawnBun with the computed remainingBatchTimeout.
- Around line 428-441: The parallel-batch branch currently moves all
isParallelSafe(t) tests into parallelBatchTests, which loses per-file
exception/leak validation semantics; modify the logic that populates
parallelBatchTests (the block guarded by options["parallel-batch"]) to remove
any tests that appear in skipsForExceptionValidation and skipsForLeaks from
parallelBatchTests (i.e., compute parallelBatchTests =
parallelBatchTests.filter(t => !skipsForExceptionValidation.has(t) &&
!skipsForLeaks.has(t))), keep those removed tests in the sequential list, and
ensure the shared batch environment enables the appropriate validation flags for
the remaining parallelBatchTests; apply the same change in the equivalent code
region around lines 1400-1414.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0546dba4-4c0f-4e19-9fd4-6a65fa1859be
📒 Files selected for processing (1)
scripts/runner.node.mjs
| // ASAN keeps the per-file path: LSAN reports at process exit, and --parallel | ||
| // workers run many files per process under --isolate, so a batch leak report | ||
| // can't be attributed to a file. Exception validation alone isn't worth the | ||
| // coverage trade. | ||
| if (options["parallel-batch"] && !basename(execPath).includes("asan")) { | ||
| const sequential = []; | ||
| for (const t of tests) { | ||
| if (isParallelSafe(t)) parallelBatchTests.push(t); | ||
| else sequential.push(t); | ||
| } | ||
| tests = sequential; | ||
| !isQuiet && | ||
| console.log("Parallel batch:", parallelBatchTests.length, "files; sequential tail:", tests.length, "files"); | ||
| } |
There was a problem hiding this comment.
--parallel-batch currently weakens local validation coverage.
The sequential path intentionally enables exception/leak validation on local runs (!isCI) and honors the per-file test/no-validate-*.txt denylists. The new batch path is still enabled locally, but its shared env cannot preserve those per-file semantics, so batched files now run with materially different diagnostics than the existing per-file path.
🛠️ Minimal safe fix
- if (options["parallel-batch"] && !basename(execPath).includes("asan")) {
+ if (options["parallel-batch"] && isCI && !basename(execPath).includes("asan")) {If local batching is still required, the safer alternative is to subtract skipsForExceptionValidation and skipsForLeaksan from parallelBatchTests before batching, then enable the shared validation env for the remaining batch.
Also applies to: 1400-1414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/runner.node.mjs` around lines 428 - 441, The parallel-batch branch
currently moves all isParallelSafe(t) tests into parallelBatchTests, which loses
per-file exception/leak validation semantics; modify the logic that populates
parallelBatchTests (the block guarded by options["parallel-batch"]) to remove
any tests that appear in skipsForExceptionValidation and skipsForLeaks from
parallelBatchTests (i.e., compute parallelBatchTests =
parallelBatchTests.filter(t => !skipsForExceptionValidation.has(t) &&
!skipsForLeaks.has(t))), keep those removed tests in the sequential list, and
ensure the shared batch environment enables the appropriate validation flags for
the remaining parallelBatchTests; apply the same change in the equivalent code
region around lines 1400-1414.
Each argv chunk now gets the remaining wall-clock budget, and the loop breaks once the budget is exhausted so unreached files fall through to per-file retry. Only matters when chunking kicks in (Windows argv limit).
| ...chunk, | ||
| ]; | ||
|
|
||
| const env = { GITHUB_ACTIONS: "true" }; |
There was a problem hiding this comment.
🟡 The batch env is now hardcoded to { GITHUB_ACTIONS: "true" }, so a local (!isCI) non-ASAN run with --parallel-batch no longer gets BUN_JSC_validateExceptionChecks=1 / BUN_JSC_dumpSimulatedThrows=1, while the per-file path in spawnBunTest() still sets them under (asan || !isCI). Commit 6390a25 gated ASAN out of the batch (so the comment about LSAN attribution is correct for ASAN), but it dropped the !isCI half as collateral — either add if (!isCI) { env.BUN_JSC_validateExceptionChecks = "1"; env.BUN_JSC_dumpSimulatedThrows = "1"; } here, or note that local --parallel-batch trades exception-validation for speed.
Extended reasoning...
What the bug is
spawnBunTestParallelBatch() builds the batch environment as:
const env = { GITHUB_ACTIONS: "true" };with no exception-validation vars. The per-file path in spawnBunTest() (scripts/runner.node.mjs:~1511) does:
if ((basename(execPath).includes("asan") || !isCI) && shouldValidateExceptions(...)) {
env.BUN_JSC_validateExceptionChecks = "1";
env.BUN_JSC_dumpSimulatedThrows = "1";
}The batch entry gate at line ~432 is options["parallel-batch"] && !basename(execPath).includes("asan") — it does not check isCI. So a local non-ASAN debug build (e.g. ./build/debug/bun-debug) with --parallel-batch enters the batch and runs every batched file without BUN_JSC_validateExceptionChecks, whereas the same files run via the sequential per-file loop would have it enabled. The runner's spawnSafe() explicitly scans output for ERROR: Unchecked JS exception: and fails the test on it, so this is real coverage that the batch path silently drops.
Why this looks like an oversight, not a deliberate trade
The PR timeline shows an earlier revision had if (isAsan || !isCI) { env.BUN_JSC_validateExceptionChecks = ... } in the batch, plus a needsPerFileEnv predicate that routed exception-validation-denylisted tests to the tail. Commit 6390a25 removed that block when it switched ASAN to skip the batch entirely. The new comment at lines 427–430 says:
ASAN keeps the per-file path: LSAN reports at process exit, and --parallel workers run many files per process under --isolate, so a batch leak report can't be attributed to a file. Exception validation alone isn't worth the coverage trade.
That sentence is justifying why ASAN binaries skip the batch (LSAN attribution is the real reason; exception validation alone wouldn't justify it). It is ASAN-specific reasoning placed in the ASAN-gating block — it doesn't read as documentation that local non-ASAN runs should also lose exception validation. The !isCI half of the original (isAsan || !isCI) condition was simply dropped along with the ASAN refactor without a replacement.
(Addressing the refutation directly: yes, the comment exists, but "Exception validation alone isn't worth the coverage trade" is explaining why ASAN doesn't batch just for exception validation — it's not saying local devs should lose exception validation. If it were intended to cover !isCI it would be odd to phrase it as part of the ASAN rationale and to leave the per-file path still setting these vars under !isCI.)
Step-by-step proof
- Local machine,
isCI = false. Runnode scripts/runner.node.mjs --exec-path ./build/debug/bun-debug --parallel-batch. basename(execPath)=bun-debug→ doesn't includeasan→ batch gate at line ~432 is satisfied.js/bun/util/inspect.test.js(say) is parallel-safe → goes intoparallelBatchTests.spawnBunTestParallelBatch()spawns it withenv = { GITHUB_ACTIONS: "true" }only.- Without
--parallel-batch, the same test reachesspawnBunTest()where(false || !isCI)→true, so it runs withBUN_JSC_validateExceptionChecks=1/BUN_JSC_dumpSimulatedThrows=1. - Net: any "ERROR: Unchecked JS exception" that the per-file path would have caught is invisible in the batch run.
Impact
Local-only, opt-in flag (default false), and the PR's primary target is CI (where !isCI is false anyway). BUN_DESTRUCT_VM_ON_EXIT / ASAN_OPTIONS / LSAN_OPTIONS are no-ops on non-ASAN builds, so exception validation is the only thing actually lost. Still, it's a real behavioral difference between the two code paths for the documented bun run test workflow (package.json defines test as node scripts/runner.node.mjs --exec-path ./build/debug/bun-debug), and a developer adding --parallel-batch for speed wouldn't expect to lose unchecked-exception detection.
Fix
One line:
const env = { GITHUB_ACTIONS: "true" };
if (!isCI) {
env.BUN_JSC_validateExceptionChecks = "1";
env.BUN_JSC_dumpSimulatedThrows = "1";
}(ASAN is already excluded from the batch, so no isAsan term is needed here.) Alternatively, if the omission is intentional, extend the comment to say so explicitly for the !isCI case.
Runner already no-ops the batch on ASAN binaries; Windows stays per-file until argv chunking is validated. Also routes batch stdout/stderr through pipeTestStdout so ::error::/::group:: annotations don't print as raw lines.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/runner.node.mjs (1)
1401-1416:⚠️ Potential issue | 🟡 MinorBatch env lacks exception validation for local (non-CI) runs.
The sequential path enables
BUN_JSC_validateExceptionCheckswhen!isCI(local runs), but the batch env only setsGITHUB_ACTIONS. While ASAN binaries are correctly excluded from batching, local non-ASAN runs lose exception validation coverage.Consider adding exception validation for local runs:
🔧 Suggested fix
const env = { GITHUB_ACTIONS: "true" }; + // Enable exception validation for local runs (matching sequential path behavior) + if (!isCI) { + env.BUN_JSC_validateExceptionChecks = "1"; + env.BUN_JSC_dumpSimulatedThrows = "1"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/runner.node.mjs` around lines 1401 - 1416, The batch run only sets GITHUB_ACTIONS in env, so local (non-CI) runs lose the BUN_JSC_validateExceptionChecks flag used in sequential mode; modify the env object passed to spawnBun in the batching path to also set BUN_JSC_validateExceptionChecks when isCI is false (i.e., preserve the same conditional used elsewhere), e.g. compute env from { GITHUB_ACTIONS: "true" } and, if !isCI, add BUN_JSC_validateExceptionChecks: "true" before calling spawnBun in the block that logs batch info and invokes spawnBun (referencing variables/env and function spawnBun, and flag name BUN_JSC_validateExceptionChecks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/runner.node.mjs`:
- Around line 1401-1416: The batch run only sets GITHUB_ACTIONS in env, so local
(non-CI) runs lose the BUN_JSC_validateExceptionChecks flag used in sequential
mode; modify the env object passed to spawnBun in the batching path to also set
BUN_JSC_validateExceptionChecks when isCI is false (i.e., preserve the same
conditional used elsewhere), e.g. compute env from { GITHUB_ACTIONS: "true" }
and, if !isCI, add BUN_JSC_validateExceptionChecks: "true" before calling
spawnBun in the block that logs batch info and invokes spawnBun (referencing
variables/env and function spawnBun, and flag name
BUN_JSC_validateExceptionChecks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03f313f3-918e-4a40-abdc-128233d03c51
📒 Files selected for processing (2)
.buildkite/ci.mjsscripts/runner.node.mjs
| // the file ran to completion — a fragment that crashed mid-file may carry | ||
| // inner describe suites but no closed file-level one. | ||
| const suiteRe = /<testsuite\s+name="([^"]+)"\s+file="([^"]+)"[^>]*\bfailures="(\d+)"/g; | ||
| const failedSuffixes = new Set(); | ||
| const seenSuffixes = new Set(); | ||
| let m; | ||
| while ((m = suiteRe.exec(xml))) { | ||
| if (m[1] !== m[2]) continue; | ||
| const file = m[2].replaceAll("\\", "/"); | ||
| const failures = parseInt(m[3]); | ||
| seenSuffixes.add(file); | ||
| if (failures > 0) failedSuffixes.add(file); | ||
| } | ||
| const matchesSuffix = (set, p) => { | ||
| for (const s of set) if (s === p || s.endsWith("/" + p) || p.endsWith("/" + s)) return true; | ||
| return false; | ||
| }; | ||
| const passed = []; | ||
| for (const p of testPaths) { | ||
| const posix = p.replaceAll(sep, "/"); | ||
| if (matchesSuffix(failedSuffixes, posix)) continue; | ||
| // A file the batch never reached (coordinator crash / timeout) won't have | ||
| // a suite at all — treat as not-passed so it falls through to retry. | ||
| if (!matchesSuffix(seenSuffixes, posix)) continue; | ||
| passed.push(p); | ||
| } |
There was a problem hiding this comment.
🔴 A file where every test() passes but an error is thrown between tests (e.g. a setTimeout callback or a sync describe() body throw after at least one test ran) gets <testsuite name=F file=F failures="0"> in the merged JUnit — bun's JunitReporter never reflects unhandled_errors_between_tests in failures=, it only forces exit code 1. parsePassedFilesFromJunit() therefore marks the file passed and it's pushed to okResults with no retry, whereas the per-file path would see exit code 1 → ok:false → fail/retry. Since ci.mjs now passes --parallel-batch on all non-Windows shards, this is a live false-green path; the PR description's "unhandled-error files don't get a file= suite" only holds when the error fires before any test runs.
Extended reasoning...
What the bug is
parsePassedFilesFromJunit() (scripts/runner.node.mjs:1438-1463) treats a file as passed iff its file-level <testsuite> (the one where name === file) has failures="0". But bun's JunitReporter populates that attribute only from per-test .fail* statuses; the separate unhandled_errors_between_tests counter — incremented for errors thrown outside any test() body after the file has started running — is never reflected in JUnit output. It only forces exit_code = 1. The batch path discards the spawnBun return value and consults the JUnit XML alone, so this exit code 1 is invisible to it.
Code path
- JUnit
failures=source:JunitReporter.endTestSuite()(src/cli/test_command.zig:349-357) writesfailures="{d}"fromsuite_info.metrics.failures. That field is incremented only inwriteTestCase()for.fail/.fail_because_*statuses (test_command.zig:420, 436, 449, 462, 475, 508). Noerrors=attribute is emitted. unhandled_errors_between_tests: a separate field on the jest runner (src/bun.js/test/jest.zig:105), incremented at src/bun.js/test/bun_test.zig:765 for.show_unhandled_error_between_tests/.show_unhandled_error_in_describe. Never touchesJunitReporter.--parallelworker: runs the file viaTestCommand.run(src/cli/test/parallel/runner.zig:344), then sends afile_doneIPC frame including the unhandled-error delta (runner.zig:360). It writes its JUnit fragment via the sameJunitReporterthat never saw the unhandled error.- Coordinator:
mergeJUnitFragments(src/cli/test/parallel/aggregate.zig:21-42) concatenates worker fragment bodies verbatim. It injects synthetic failing<testsuite>entries only forcoord.crashed_files(aggregate.zig:44-60) — i.e. worker process death — not for files withunhandled > 0. The unhandled count from IPC lands inreporter.jest.unhandled_errors_between_tests(Coordinator.zig:248), which only affects the final exit code at test_command.zig:2025-2026. - Runner:
spawnBunTestParallelBatch()doesawait spawnBun(...)without binding the result (runner.node.mjs:1408), then callsparsePassedFilesFromJunit()which sees<testsuite name="test/foo.test.ts" file="test/foo.test.ts" ... failures="0">, matchesm[1] === m[2], parsesfailures = 0, and adds it toseenSuffixesonly → returned as passed. The caller pushes it intookResultsand never re-runs it.
Why the PR's stated guard doesn't apply
The PR description says "Crashed workers and unhandled-error files don't get a file= suite, so they fall through to per-file retry". That's true only for two sub-cases: (a) the worker process crashes (synthetic suite with failures="1" is injected for crashed_files), and (b) the unhandled error fires at module-load time before any test() has registered/run (then no file-level suite is opened, so seenSuffixes won't contain it). But when at least one test() passes and an exception then surfaces between tests / after the last test / in a describe() body, the file-level suite is opened and closed with failures="0", and that's the case this code mis-classifies.
Step-by-step proof
Given test/js/foo.test.ts:
import { test } from "bun:test";
test("a", () => {});
setTimeout(() => { throw new Error("boom"); }, 0);- Per-file path (no
--parallel-batch):spawnBunTest()→bun testruns test "a" (pass), then the timer fires →unhandled_errors_between_tests = 1→ exit code 1 (test_command.zig:2025).spawnSafeseesexitCode = 1, no "N fail" line in stderr →error = "code 1",ok = false. File is reported failing and retried. - Batch path (
--parallel-batch): worker runs the file, opens file-level<testsuite>, writes<testcase name="a">(pass), closes the suite withfailures="0". Timer fires → worker'sunhandled_errors_between_tests++. Worker sendsfile_donewithunhandled = 1over IPC, flushes its JUnit fragment unchanged. Coordinator merges fragments verbatim (file is not incrashed_files), accumulatesunhandled = 1, exits 1. spawnBunTestParallelBatchignores the exit code, readsbatch-0.xml, regex matches<testsuite name="test/js/foo.test.ts" file="test/js/foo.test.ts" tests="1" assertions="0" failures="0" ...>.m[1] === m[2],failures = 0→ added toseenSuffixes, not tofailedSuffixes.- Back in
runTests():passed.has("js/foo.test.ts")→ pushed tookResults, removed from the retry queue. CI shard goes green.
The PR test plan checked "unhandled rejection → retry", but that almost certainly exercised a rejection during a test body (produces .fail → failures>0) or at module load (no suite emitted). The between-tests / post-test case is the gap.
Impact
This violates the function's documented invariant — "this never has to be precise about why a file failed, only whether it definitely passed" — in the dangerous direction: a file that the existing per-file path would flag red is silently marked green. Since this PR also flips --parallel-batch on for every non-Windows test-bun step in .buildkite/ci.mjs, it goes live immediately rather than being opt-in.
Fix
Either side works:
- bun side (cleanest): when a worker reports
unhandled > 0for a file, havemergeJUnitFragmentsinject a synthetic failing<testcase>into that file's suite (or just append a synthetic<testsuite name=F file=F failures="1">) the same way it already does forcrashed_files. Then the JUnit reflects reality andparsePassedFilesFromJunitneeds no change. - runner side (local to this PR): capture the
spawnBunresult and, ifexitCode !== 0, additionally require some independent confirmation before trustingfailures="0"— e.g. don't mark any file passed unless the batch exited 0, or parse stderr for the# Unhandled error between testsbanner. The simplest correct version is: if the batch exited non-zero and zero files were classified as failed, fall every batched file through to per-file retry (slow but safe), since a non-zero exit with no JUnit failures is exactly this signature.
| len += arg.length + 1; | ||
| } | ||
|
|
||
| const perTestTimeout = Math.ceil(testTimeout / 2); |
There was a problem hiding this comment.
🟡 nit: The batch hardcodes --timeout=${Math.ceil(testTimeout/2)} = 90s, while the per-file path uses getTestTimeout(testPath)/2 which yields 150s for paths matching /integration|3rd_party|docker|bun-install-registry|v8|bundler_compile/i. test/integration/** and test/bundler/bundler_compile*.test.ts aren't in no-parallel.txt, so any test() in those files relying on the runner default in the 90–150s window will time out in the batch, fall through to per-file retry, then pass — self-healing but wasted wall-clock. Math.ceil(integrationTimeout/2) would match the per-file ceiling without routing whole directories to the tail.
Extended reasoning...
What the divergence is
spawnBunTestParallelBatch() computes the per-test default timeout as:
const perTestTimeout = Math.ceil(testTimeout / 2); // 180_000 / 2 = 90_000and passes it as --timeout=90000 for the entire batch. The per-file path in spawnBunTest() instead does:
const timeout = getTestTimeout(testPath); // 300_000 for integration paths
const perTestTimeout = Math.ceil(timeout / 2); // 150_000where getTestTimeout() (scripts/runner.node.mjs:1559) returns integrationTimeout (5 min → 150s per-test) for paths matching /integration|3rd_party|docker|bun-install-registry|v8|bundler_compile/i. So the same test file gets a 90s default in the batch but a 150s default when run per-file.
Which files are affected
Checked test/no-parallel.txt: it contains no entries for integration/, bundler_compile, docker, or bun-install-registry (only v8/ is path-excluded via PATH_PATTERNS in the generator). So test/integration/** (esbuild, expo-app, jsdom, mysql2, next-pages, …) and test/bundler/bundler_compile*.test.ts all enter the batch with the 90s default. Note that --timeout is only the default per-test timeout — tests with an explicit third argument (e.g. test('next build', fn, 600_000)) are unaffected, so this only bites test() calls that rely on the runner-supplied default and legitimately take 90–150s.
Step-by-step
- CI shard runs with
--parallel-batch(now on by default for non-Windows per.buildkite/ci.mjs:706), non-ASAN binary. test/bundler/bundler_compile.test.tsis not inno-parallel.txt→isParallelSafe()returns true → goes intoparallelBatchTests.- Batch spawns
bun test --parallel --timeout=90000 ... ./test/bundler/bundler_compile.test.ts .... - Suppose one
test()in that file has no explicit timeout and takes 110s on this shard.bun testmarks it.fail_because_timeout→ JUnit<testsuite ... failures="1">. parsePassedFilesFromJunit()seesfailures>0→ file not inpassed→ re-queued intotestsahead of the sequential tail.- Per-file retry hits
spawnBunTest()→getTestTimeout('test/bundler/bundler_compile.test.ts')matchesbundler_compile→ 300_000 →--timeout=150000→ test passes at 110s. runTest()'s attempt counter starts at 1 for the per-file run, so the file lands inokResults(not evenflakyResults).
Net effect: correctness is preserved, but the batch burned ~90s on a doomed run and the file then runs again sequentially.
Addressing "this is the batch working as designed"
It's true the batch's docstring says it "never has to be precise about why a file failed — only whether it definitely passed", and the per-file fallback is the intended safety net. So this isn't a correctness bug. And routing all getTestTimeout()-elevated directories to the sequential tail would indeed defeat the speedup for the many fast tests in integration/ — that's not the right fix.
But the cheap fix has no such downside: const perTestTimeout = Math.ceil(integrationTimeout / 2); gives every batched test the same 150s default ceiling the per-file path would. The only cost is that a genuinely hung test (no explicit timeout, never completes) now waits 150s instead of 90s before failing — but that's already what happens on the per-file retry, and the whole batch is still bounded by --parallel-batch-timeout (30 min). The benefit is that the two code paths agree on what "default timeout" means, and slow-but-passing integration/bundler_compile tests don't generate spurious batch failures + sequential reruns.
Impact
Low — self-healing, opt-in flag (though now default-on for non-Windows CI), and most slow tests in these directories already declare explicit timeouts. Filing as a nit because it's a real semantic divergence between the two paths with a one-token fix.
| await spawnBun(execPath, { | ||
| args, | ||
| cwd, | ||
| timeout: remaining, | ||
| env, | ||
| stdout: chunk => pipeTestStdout(process.stdout, chunk), | ||
| stderr: chunk => pipeTestStdout(process.stderr, chunk), | ||
| }); |
There was a problem hiding this comment.
🟡 nit: The await spawnBun(...) return value is discarded, so result.crashes — the GDB backtrace from any new core dump plus the symbolicated trace fetched from the ci-remap server's /traces endpoint — is computed and then thrown away. The per-file path (spawnBunTest, node-test branch) appends crashes to stdoutPreview so it lands in the Buildkite annotation; here it's just lost, and since the /traces fetch drains the remap server's queue, a flaky one-off crash in the batch leaves no symbolicated trace anywhere in the build log. Correctness is unaffected (failures fall through to per-file retry, core files survive for the tarball), but a one-liner const r = await spawnBun(...); if (r.crashes) process.stderr.write(r.crashes); would preserve the diagnostics.
Extended reasoning...
What the bug is
spawnBunTestParallelBatch() calls await spawnBun(execPath, {...}) without binding the return value. spawnBun() returns { ok, error, stdout, crashes }, where crashes is computed after the subprocess exits and contains two things that are not in the live stdout/stderr stream:
- GDB backtraces — when
options['coredump-upload']is on (default for Buildkite+Linux),spawnBundiffscoresDirbefore/after, runsgdb -batch --eval-command=bt --core <core> <execPath>on each new core file, and accumulates the filtered backtrace intocrashes. - Remapped crash traces — when
remapPortis set (it is started inrunTests()before the batch runs) andexitCode !== 0,spawnBunfetcheshttp://localhost:${remapPort}/tracesfrom the ci-remap server and appends the symbolicated traces tocrashes.
Every other call site that runs tests captures this value: the node-test branch in runTests() does const { ok, error, stdout, crashes } = await spawnBun(...) and appends crashes to stdoutPreview; spawnBunTest() does the same; spawnBunInstall() appends it to stdout. The batch is the only path that drops it.
Why this loses information that can't be recovered later
The live stdout/stderr of the batch is piped to the log via pipeTestStdout, so bun's own panic message ("oh no: Bun has crashed", the raw tracestring) is visible. What's lost is the post-mortem GDB backtrace and the symbol-remapped trace text — neither of those is part of subprocess output.
The GDB loss is partially recoverable: the core file stays on disk in coresDir and is uploaded in the end-of-run encrypted tarball, so a human can re-run gdb offline. But the inline backtrace that would tell you "batch chunk 0 segfaulted in WTF::StringImpl::deref at frame #3" without downloading a 2 GB tarball is gone. Also, because the new core is added to existingCores for the next spawnBun call, it won't be re-gdb'd by any later per-file run — this was the only place that backtrace would have been printed.
The remap loss is worse: per the comment at scripts/runner.node.mjs:~1320, the /traces fetch consumes the server's queue ("crash reports will instead be attributed to the next test that fails"). So spawnBun fetches the symbolicated trace, puts it in result.crashes, the batch discards result, and the trace is gone — it won't show up again when the file is retried per-file unless the crash reproduces.
Step-by-step proof
- Buildkite Linux job opts into
--parallel-batch.isBuildkite && isLinux→options['coredump-upload'] = true,coresDiris set.runTests()starts the ci-remap server →remapPort = 12345. spawnBunTestParallelBatch()spawnsbun test --parallel ...for chunk 0. A worker segfaults onjs/web/fetch/foo.test.ts.- The worker writes
/var/bun-cores-.../bun-12345.coreand POSTs its tracestring tohttp://localhost:12345. The coordinator emits a synthetic<testsuite>for the crashed worker (withoutfile=, so it won't matchparsePassedFilesFromJunit) and exits non-zero. - Inside
spawnBun:newCores = ['bun-12345.core']→ runs gdb, accumulates ~20 lines of backtrace intocrashes.exitCode !== 0→ fetches/traces, gets[{remap: "..."}], appends it tocrashes. Returns{ ok: false, error: 'core dumped', crashes: '======== Stack trace from GDB ...' }. - Back in
spawnBunTestParallelBatch:await spawnBun(...)— return value discarded. The GDB output and remapped trace are garbage-collected. foo.test.tshas no file-level<testsuite>→ not inpassed→ falls through to per-file retry. If the crash was flaky and doesn't reproduce, the retry passes and the only evidence the crash ever happened is the raw panic line in the batch's interleaved stdout plus a core file in the end-of-run tarball.
Why existing code doesn't prevent it
The function's docstring explicitly says "this never has to be precise about why a file failed — only whether it definitely passed", so dropping diagnostics is consistent with the stated contract. Correctness (pass/fail attribution) is fully preserved by the JUnit parse + per-file fallback. This is purely a diagnostic-quality gap, which is why it's a nit rather than a blocker.
Impact
- Correctness: none. Crashed/unrun files always fall through to per-file retry.
- Reproducible crashes: minimal — they re-crash on per-file retry with full
crashescontext attached to the Buildkite annotation. - Flaky/one-off crashes: the inline GDB backtrace and the symbolicated remap are silently lost from the build log. The core file survives in the encrypted tarball, but that's a much higher-friction debug path than reading the annotation.
Fix
One line:
const result = await spawnBun(execPath, { ... });
if (result.crashes) process.stderr.write(result.crashes);(or fold it into the existing !isQuiet && console.log block below).
…it report (#37483) ### Problem `scripts/runner.node.mjs` runs the parallel-safe bucket as one `bun test --parallel --reporter=junit` (#36175) and reads the report to decide which files failed, what to re-run alone, and the `(X.XXs)` it prints per file. It collected the suites like this: ```js for (const [, attrs] of xml.matchAll(/<testsuite\b([^>]*)>/g)) { const file = /\bfile="([^"]+)"/.exec(attrs)?.[1]; ... if (file) suites.set(file, { failures, seconds, cases: [] }); } ``` bun's reporter writes one `<testsuite name="<path>" file="<path>">` per file and, nested inside it, one `<testsuite name="<describe>" file="<path>">` per describe block (`begin_test_suite_with_line` in `src/runtime/cli/test_command.rs` puts `file=` on both kinds). Every suite of a file therefore overwrote the previous one, and a file with describe blocks ended up represented by whichever describe suite came last in the report. Report written by the released bun for a file with a failing top-level test, a failing `describe("first")` and a passing `describe("second")`, plus a file with a `describe.concurrent` of three 100ms tests: ```xml <testsuite name="test/a.test.ts" file="test/a.test.ts" tests="4" failures="2" time="0.002066623" ...> ... <testsuite name="first" file="test/a.test.ts" tests="1" failures="1" time="0" ...> <testsuite name="second" file="test/a.test.ts" tests="2" failures="0" time="0" ...> </testsuite> <testsuite name="test/b.test.ts" file="test/b.test.ts" tests="3" failures="0" time="0.100953121" ...> <testsuite name="conc" file="test/b.test.ts" tests="3" failures="0" time="0.3" ...> </testsuite> ``` The runner's loop turned that into `a.test.ts: { failures: 0, seconds: 0 }` and `b.test.ts: { seconds: 0.3 }`: * `a.test.ts` is not added to `failed`, and because the report exists (`evidence` is true) nothing is re-run: the file is printed as passed and pushed to `okResults`, so the bucket's non-zero exit is swallowed and the job goes green. Any bucket file whose failures are at top level or in a describe block other than the last one is affected. * The per-file time is a describe block's sum of test durations instead of the file suite's wall clock (`end_test_suite` times file suites from `file_start_ns` to `file_end_ns` and describe suites by summing their tests), so `describe.concurrent` files over-report and files with several describes under-report. These lines feed `scripts/ci-slowest-tests.ts` and `scripts/update-test-durations.mjs`. * The failing cases are attached to that same entry, and since the file is not classified as failed they are never printed in the "failing in the parallel batch" group or the flaky annotation. ### Fix The parsing moves to `parseJunitFileSuites()` in `scripts/utils.mjs` (the runner's existing helper module; `runner.node.mjs` itself runs on import, so it cannot be unit tested) and only keeps suites whose `name` is their `file`, which is how the reporter writes file suites. That is the right suite to read on its own terms: `end_test_suite` adds each closed suite's metrics to its parent, so the file suite's `failures` includes every nested describe block, and its `time` is the only wall clock measurement in the report. The synthetic suite the `--parallel` coordinator writes for a file whose worker crashed or was interrupted (`merge_junit_fragments` in `src/runtime/cli/test/parallel/aggregate.rs`) has the same `name == file` shape, so crashed and hung files are still classified as failed. The `<testcase>` loop is unchanged: it keys on each case's own `file` attribute. The runner's classification, printing and annotation code is unchanged apart from calling the helper. #36218 (in flight) keeps the same file-suite shape, so it is unaffected. The earlier draft of the batch runner, #29654, applied this same `name === file` rule in its `parsePassedFilesFromJunit()`; the rule did not make it into the version that landed in #36175. #29654 is a rewrite of the whole batch mechanism and conflicts with main, so this PR only restores the rule in the code that shipped. ### Tests `test/internal/runner-junit.test.ts`: * a hand-written report in the reporter's shape (top-level failure, a failing describe, a passing describe with a nested describe last, a second plain file): the file is reported with `failures: 2`, the file suite's `time`, and both failing cases with their names and messages unescaped; * Windows-style backslash paths and escaped characters are keyed the way the runner looks files up; * the coordinator's crashed-file suite is reported as one failure with no cases; * a real `bun test --parallel=2 --reporter=junit` run over a file with describe blocks, a plain file and a file that calls `process.exit()`: the report contains four suites carrying the describe file's path, and the parsed result has the file suite's counts, its `time`, the two failing cases, and the crash marker. With the previous behaviour (every suite with a `file` attribute) the first and last of these fail (`failures: 0` and the last describe block's time for the file with describe blocks); with this change all five pass under `bun bd test` and with the released bun. `node scripts/runner.node.mjs --exec-path ./build/debug/bun-debug js/web/encoding` still forms the 8-file bucket, prints the file suites' times, and re-ran the one file the interrupted batch marked as failed.
|
Closing this as superseded.
Reopen if something from this branch is still missing on main. |
2 participants
Summary
scripts/runner.node.mjs --parallel-batchruns all parallel-safe test files in onebun test --parallelinvocation first, parses the merged JUnit for per-file pass/fail, then feeds failures + the denylist through the existing per-file retry/annotation loop. The flag defaults to off so this is a no-op until a CI job opts in.test/no-parallel.txtis the single source of truth (214 entries).isParallelSafe()does only the .txt lookup plus the structural check that node-test/cluster files run viabun runrather thanbun test. No path heuristics in the runner.bun run regenerate-no-parallelrewrites the auto section. A file is denylisted when its path is undernapi/,v8/,ffi/, orwebview/; its basename containsleak|stress|memory|heap|gc|rss; or its body references GC/RSS/heap measurement (expectMaxObjectTypeCount,Bun.gc(,gcTick,heapStats,generateHeapSnapshot,process.memoryUsage,.rss,heapUsed,Bun.WebView,withoutAggressiveGC,BUN_JSC_forceRAMSize). Hand-added entries go below# manualand survive regeneration. Also exposed as/regenerate-no-parallel.<testsuite>hasfailures="0"and no suite for it hasfailures>0. Crashed workers and unhandled-error files don't get afile=suite, so they fall through to per-file retry — the batch never has to be precise about why something failed.--parallelflag and the vendoredp-limit.mjs/yocto-queue.mjs; the per-file loop is now a plain sequentialfor…of.Test plan
node --check scripts/runner.node.mjs--parallel-batch: per-file behavior unchangedno-parallel.txtonly (napi/v8/leak/content-match all from the file)bun run regenerate-no-parallel→ 214 entries, preserves# manual