Repository navigation
make some tests faster - #33622
make some tests faster#33622
Conversation
|
Updated 7:27 AM PT - Jul 7th, 2026
❌ @robobun, your commit 584bb9f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33622That installs a local version of the PR into your bun-33622 --bun |
|
@robobun adopt this PR and check the end result is CI tests for linux finish faster for the changed tests than they did in other recent builds |
|
Adopted. Build #69787 (584bb9f, all linux+windows lanes complete): zero error-level test failures. The only failed job is the recurring darwin
linux-x64-debian shard spread is now 146s..196s (50s) vs baseline 86s..308s (222s). 7 retry-recovered flakes in this build, all at or below their baseline rate across 10 recent builds; none of the flakes the earlier commits introduced ( Stack: within-file concurrency + LPT bin-pack sharding over Ready for review; the darwin lane needs that agent looked at. |
|
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 (6)
💤 Files with no reviewable changes (1)
WalkthroughThis PR overhauls the test runner and CI infrastructure to schedule tests via deterministic LPT bin-packing using historical duration data, introduces a shared Docker prestart-map consumed by both the runner and coordinator, adds a duration-tracking pipeline, and converts large swaths of test suites to run concurrently while adjusting numerous timeouts, iteration counts, and payload sizes. It also refactors N-API addon build harnesses to add a fast compile path and relocates/reduces regression test fixtures. ChangesTest Runner and Docker Infrastructure
Test Duration Tracking Pipeline
Test Suite Concurrency and Timing
N-API Addon Build Fixture Overhaul
Regression Test Refactors
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/node/process/dlopen-duplicate-load.test.ts (2)
96-234: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
test.concurrentfor these subprocess-spawning tests.Every test here spawns an independent
Bun.spawnsubprocess against a shared, read-onlyaddonPath, matching the repo convention that subprocess/file-I/O tests are goodtest.concurrentcandidates — directly aligned with this PR's "make tests faster" objective.Based on learnings, "treat a use of
describe.concurrent/test.concurrentas a concurrency guideline only when the tests spawn subprocesses and/or perform filesystem I/O," which applies here since all five tests spawn subprocesses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/process/dlopen-duplicate-load.test.ts` around lines 96 - 234, These subprocess-based tests in process.dlopen duplicate loads and process.dlopen with non-object exports are independent and should run concurrently to match the repo’s test-speedup pattern. Update the individual test cases under the describe blocks to use test.concurrent, since each one spawns its own Bun.spawn process and only reads the shared addonPath without mutating shared state. Keep the existing assertions and structure, but apply concurrency at the test declaration level for the duplicate-load and non-object-exports cases.Source: Learnings
114-233: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract a shared spawn helper; add the missing stderr diagnostic on failure.
The
Bun.spawn+Promise.all([...stdout, stderr, exited])block is duplicated five times, along with theaddonPath.replace(/\\/g, "\\\\")escaping. None of the five tests surfacestderrwhen the subprocess fails — onlystdout/exitCodeare asserted, so a crash/failure won't show diagnostic output in the failure diff.♻️ Proposed helper extraction
+ const escapedAddonPath = addonPath.replace(/\\/g, "\\\\"); + + async function runDlopenScript(testScript: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", testScript], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + if (exitCode !== 0) { + expect(stderr).toBe(""); + } + return { stdout, exitCode }; + }Then each test becomes:
- await using proc = Bun.spawn({ - cmd: [bunExe(), "-e", testScript], - env: bunEnv, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const { stdout, exitCode } = await runDlopenScript(testScript);Based on learnings, "when you spawn a subprocess in a bun:test and you assert on its exit code, follow the CLAUDE.md house style: write
if (exitCode !== 0) { expect(stderr).toBe(\"\"); }immediately beforeexpect(exitCode).toBe(0)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/process/dlopen-duplicate-load.test.ts` around lines 114 - 233, The subprocess setup in these `process.dlopen` tests is duplicated and the failure path never reports `stderr`, making debugging hard. Extract the repeated `Bun.spawn`/`Promise.all` pattern into a shared helper and centralize the `addonPath` escaping there, then update the tests that assert `exitCode` in the `dlopen` suite to include the house-style `stderr` check before asserting success so failures surface diagnostics; use the existing `Bun.spawn`, `proc.exited`, and `addonPath.replace` patterns to locate the shared setup.Source: Learnings
test/js/bun/spawn/spawn.test.ts (1)
168-213: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReduce the concurrent Bun batch under ASAN/debug. This spawns 50
bunExe()processes per batch; lowerbatchSizeto 5 whenisASAN || isDebugis true so this test stays within the default timeout on slower debug+ASAN runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/bun/spawn/spawn.test.ts` around lines 168 - 213, Reduce the concurrency in the spawn stress test by making the batch size conditional in the runPair loop: keep the current 25 for normal runs, but lower it to 5 when isASAN || isDebug is true so the Promise.all batching in spawn.test.ts finishes within the default timeout on slower environments.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/cli.test.ts`:
- Around line 47-53: The temp directory cleanup in testCompileAndExec only
happens after testCompile and testExec succeed, so a failed assertion can skip
fs.rmSync and leak the tmp dir. Update testCompileAndExec to always clean up by
moving the fs.rmSync(baseDir, ...) call into a finally block around the existing
testCompile/testExec flow, keeping the same happy-path behavior while ensuring
cleanup on failure.
- Around line 37-46: The shared testExec helper in cli.test.ts is too strict by
asserting stderr is exactly empty, which can flake on ASAN/debug builds. Update
the helper used by both --outfile tests to avoid toBeEmpty() on stderr and
instead tolerate benign warnings while still checking the process exit status
from Bun.spawn. Keep the change localized to testExec so both callers benefit
from the relaxed stderr handling.
In `@test/cli/install/bun-security-scanner-matrix-runner.ts`:
- Line 570: The `runSecurityScannerTest()` flow still uses `toMatchSnapshot()`
for `sortedPackages` and `sortedTarballs` inside `test.concurrent`, which Bun
cannot handle. Update the `test.concurrent(testName, ...)` usage in
`bun-security-scanner-matrix-runner.ts` so these snapshot assertions run
sequentially, or बदल them to `toMatchInlineSnapshot()` within
`runSecurityScannerTest()` to keep the concurrent test safe.
In `@test/cli/install/bun-upgrade.test.ts`:
- Around line 217-226: The bun asset-name matrix is duplicated in both the
staging and digest tests; extract the nested-loop generation into a shared
module-level helper or constant in bun-upgrade.test.ts and reuse it from both
test cases. Keep the shared symbol close to the existing assetNames logic so the
two tests reference the same source of truth instead of building the list
independently.
In `@test/docker/coordinator.ts`:
- Around line 27-34: The `prestartMap` cast in `coordinator.ts` only changes
TypeScript types and does not validate the service names loaded from
`prestart-map.mjs`, so typos can slip through and fail silently during the
prestart loop. Add a small runtime assertion at module load around
`prestartMapRaw` that checks every mapped entry against the known `ServiceName`
set, and make it throw immediately on any unknown value. Keep the validation
close to the `prestartMap` initialization so bad entries fail fast before
`ensure()` or the `.catch(() => {})` path can hide them.
In `@test/js/node/test/parallel/test-stream-iter-broadcast-from.js`:
- Around line 103-105: Revert the change in the test-stream-iter-broadcast-from
test so it matches the upstream Node fixture exactly; the edit to the long-delay
timer in the generator should be removed from this read-only compatibility test.
If Bun needs different behavior, keep the Node-owned test unchanged and move any
local accommodation into a Bun-owned test instead, preserving the existing
structure around the generator and cancel/unblock logic.
In `@test/js/node/zlib/zlib.test.js`:
- Around line 258-260: The chunk-generation loops in the zlib tests are
zero-filling buffers before immediately overwriting them with random data.
Update both `readStream.push(randomFillSync(Buffer.alloc(...)))` call sites in
the Brotli and Zstd chunk-generation loops to use `Buffer.allocUnsafe` so the
buffers are not redundantly initialized before `randomFillSync` fills them.
In `@test/js/third_party/grpc-js/test-tonic.test.ts`:
- Around line 39-57: The tonic cache in getProtocZipBytes is never evicted, so
long-lived CI agents will keep accumulating stale protoc/Cargo artifacts under
cacheDir. Add a simple pruning or TTL strategy around the cacheDir/target
contents used by this test helper, and keep the existing cachedPath reuse
behavior while ensuring old entries are periodically removed.
In `@test/js/valkey/test-utils.ts`:
- Around line 13-17: The Valkey test enablement logic in isEnabled is
duplicating the Docker service key instead of deriving it from the shared
prestart map source of truth. Update the BUN_TEST_SERVICE_redis_unified check to
consume the same redis_unified key used by prestartMap in
test/docker/prestart-map.mjs, so any service rename stays in sync and the docker
info short-circuit remains accurate.
In `@test/napi/napi-finalizer-delete-ref.test.ts`:
- Around line 21-52: The fast path in the napi finalizer delete-ref test always
runs the node-gyp build, even when the addon output is already fresh, so add the
same freshness check used in needsInstall() from napi.test.ts and
napi-value-ffi.test.ts before invoking spawnSync for node-gyp. Gate the
configure/build path in the test’s beforeAll logic on addonPath being missing or
stale relative to the relevant sources, and only fall back to the full
install/rebuild path when the fast path does not produce the target.
In `@test/napi/napi-value-ffi.test.ts`:
- Around line 31-63: The staleness check in needsInstall() is incomplete
compared with the sibling helper in napi.test.ts because it only compares the
built .node files against each addon’s .c source. Update needsInstall() in
napi-value-ffi.test.ts to also consider binding.gyp and package.json mtimes when
deciding whether to run bun install, so changes to build config or package
metadata correctly trigger a rebuild.
In `@test/napi/node-napi-tests/harness.ts`:
- Around line 86-89: The comment in harness.ts is too long and must be reduced
to 3 lines or fewer. Shorten the explanatory block near the node-gyp
optimization logic in the harness setup so it keeps only the essential rationale
and fallback behavior, while preserving the meaning around the direct compiler
invocation and false return path.
- Around line 90-144: The per-target direct compile path in tryBuildFast can
fail before child.exited is reached if spawn() throws during setup, which
currently bypasses the node-gyp fallback. Wrap the compiler invocation and await
logic in a try/catch inside the targets.map callback, and return false on any
spawn or setup error so buildWithNodeGyp() still gets a chance to run. Keep the
fallback behavior consistent with the existing exit-code failure handling around
child.exitCode and the stderr warning.
---
Outside diff comments:
In `@test/js/bun/spawn/spawn.test.ts`:
- Around line 168-213: Reduce the concurrency in the spawn stress test by making
the batch size conditional in the runPair loop: keep the current 25 for normal
runs, but lower it to 5 when isASAN || isDebug is true so the Promise.all
batching in spawn.test.ts finishes within the default timeout on slower
environments.
In `@test/js/node/process/dlopen-duplicate-load.test.ts`:
- Around line 96-234: These subprocess-based tests in process.dlopen duplicate
loads and process.dlopen with non-object exports are independent and should run
concurrently to match the repo’s test-speedup pattern. Update the individual
test cases under the describe blocks to use test.concurrent, since each one
spawns its own Bun.spawn process and only reads the shared addonPath without
mutating shared state. Keep the existing assertions and structure, but apply
concurrency at the test declaration level for the duplicate-load and
non-object-exports cases.
- Around line 114-233: The subprocess setup in these `process.dlopen` tests is
duplicated and the failure path never reports `stderr`, making debugging hard.
Extract the repeated `Bun.spawn`/`Promise.all` pattern into a shared helper and
centralize the `addonPath` escaping there, then update the tests that assert
`exitCode` in the `dlopen` suite to include the house-style `stderr` check
before asserting success so failures surface diagnostics; use the existing
`Bun.spawn`, `proc.exited`, and `addonPath.replace` patterns to locate the
shared setup.
🪄 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: 06540f36-4b74-45af-a153-2cf45ea552cf
⛔ Files ignored due to path filters (1)
test/regression/issue/30205-napi-app/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
scripts/runner.node.mjstest/bundler/bundler_compile_autoload.test.tstest/bundler/cli.test.tstest/cli/init/init.test.tstest/cli/install/bun-install-lifecycle-scripts.test.tstest/cli/install/bun-install-proxy.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install-security-provider.test.tstest/cli/install/bun-security-scanner-matrix-runner.tstest/cli/install/bun-upgrade.test.tstest/cli/install/bun-workspaces.test.tstest/cli/install/simple-dummy-registry.tstest/cli/install/test-dev-peer-dependency-priority.test.tstest/cli/run/multi-run.test.tstest/cli/run/no-orphans.test.tstest/docker/coordinator.tstest/docker/prestart-map.mjstest/integration/next-pages/test/next-build.test.tstest/js/bun/css/color.test.tstest/js/bun/dns/resolve-dns.test.tstest/js/bun/http/bun-serve-args.test.tstest/js/bun/http/bun-serve-file.test.tstest/js/bun/http/bun-server.test.tstest/js/bun/http/http-server-chunking.test.tstest/js/bun/http/proxy-stress-concurrent.test.tstest/js/bun/http/serve-epoll-add-fail.test.tstest/js/bun/http/serve-http3.test.tstest/js/bun/http/tls-keepalive-leak-fixture.jstest/js/bun/http/tls-keepalive.test.tstest/js/bun/perf/linker-order.test.tstest/js/bun/shell/leak.test.tstest/js/bun/spawn/spawn-socketpair-shutdown.test.tstest/js/bun/spawn/spawn.test.tstest/js/bun/typescript/type-export.test.tstest/js/bun/util/password.test.tstest/js/node/net/handle-leak.test.tstest/js/node/process/dlopen-duplicate-load.test.tstest/js/node/process/process-stdin.test.tstest/js/node/test/parallel/test-dns-setserver-when-querying.jstest/js/node/test/parallel/test-stream-iter-broadcast-from.jstest/js/node/test/sequential/test-performance-eventloopdelay.jstest/js/node/tls/node-tls-cert.test.tstest/js/node/worker_threads/worker_heap_snapshot_gc.test.tstest/js/node/zlib/leak.test.tstest/js/node/zlib/zlib.test.jstest/js/sql/sql-connect-error-reporting.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql-postgres-datetime-roundtrip.test.tstest/js/sql/sql.test.tstest/js/sql/sqlite-sql.test.tstest/js/third_party/grpc-js/test-idle-timer.test.tstest/js/third_party/grpc-js/test-outlier-detection.test.tstest/js/third_party/grpc-js/test-tonic.test.tstest/js/third_party/prisma/prisma.test.tstest/js/valkey/reliability/protocol-handling.test.tstest/js/valkey/test-utils.tstest/js/valkey/unit/basic-operations.test.tstest/js/web/fetch/body-stream.test.tstest/js/web/fetch/fetch-backpressure.test.tstest/js/web/html/FormData.test.tstest/js/web/websocket/autobahn.test.tstest/js/web/websocket/websocket-syscall-fault.test.tstest/js/web/workers/structured-clone.test.tstest/napi/napi-app/binding.gyptest/napi/napi-finalizer-delete-ref.test.tstest/napi/napi-value-ffi.test.tstest/napi/napi.test.tstest/napi/node-napi-tests/harness.tstest/napi/node-napi-tests/test/js-native-api/test_constructor/do.test.tstest/napi/node-napi-tests/test/js-native-api/test_general/do.test.tstest/napi/uv.test.tstest/regression/issue/22157.test.tstest/regression/issue/26298.test.tstest/regression/issue/30205-napi-app/.gitignoretest/regression/issue/30205-napi-app/binding.gyptest/regression/issue/30205-napi-app/isolate_finalizer_addon.ctest/regression/issue/30205-napi-app/package.jsontest/regression/issue/30205.test.tstest/tsconfig.json
💤 Files with no reviewable changes (2)
- test/napi/napi-app/binding.gyp
- test/js/bun/shell/leak.test.ts
…e test edits - bun-security-scanner-matrix-runner.ts: revert to serial test() with shared registry. Bun's test runner rejects toMatchSnapshot() inside test.concurrent and this file has 1440+ snapshot entries that cannot be converted inline. - test-dns-setserver-when-querying.js: revert to upstream. The stub-server approach fails with ECONNREFUSED on the dgram socket when c-ares closes its ephemeral port after dns.setServers(), and on ASAN it exposed a null deref in dns.rs remove_timer. Files under test/js/node/test/parallel/ are upstream fixtures. - test-stream-iter-broadcast-from.js: revert to upstream for the same reason. - napi harness tryBuildFast: catch spawn() throwing (missing compiler) so the node-gyp fallback still runs instead of the whole test file erroring out.
|
@robobun can we try making only some of the known safe tests parallel? Look at test runs for the files that are quick where spawn overhead of the test itself is the main factor and let's run those in parallel. We can allowlist these test files in another test/*.txt file and then we run the parallel safe ones together? What do you think of the right way to approach parallelizing tests? You can look at timestamps from flushed BuildKite logs along with the test reported timings themselves |
|
Ran the numbers on
Two observations:
Estimated savings from 4-way file parallelism inside each of the 20 shards:
On approach,
The runner change is then: when walking An alternative I considered: batching N quick files into one Want me to implement tier 1 (node-parallel/vendor) in this PR and open a separate one for the allowlist mechanism, or do both here? Tier 1 is a ~20-line runner change with no new files. |
|
Tier 1 |
|
@robobun how do we get the wall clock time down to like 4 mins or less with half the shards? |
These ~2.8k files are parallel-safe by naming convention (Node's own CI runs test/parallel concurrently) and have a ~50ms median dominated by bun startup. A separate pLimit(min(4, nproc)) for these paths overlaps that startup without changing the serial contract for every other file. TEST_SERIAL_ID is now set for every Node-style test so test/common/tmpdir.js creates distinct .tmp.<index> directories; previously every file used .tmp.0 which only worked because they ran one at a time. Local: 63 test-buffer-* files 5.0s serial -> 1.6s at width 4.
|
Tier 1 pushed in 6c5559b (local: 63 On 4 min wall clock with 10 shards (linux-x64-debian-13, build #69493 timings, 4880 files, 3175s serial total):
(Assumes ~25s/shard of fixed overhead: clone + So three levers, in descending leverage:
Hard floor: the single slowest file ( Want me to add the LPT bin-packing (lever 1) to this PR as well? It is a contained runner change: generate |
|
@robobun yes and can we delineate between Asan and non Asan and also write a weekly BuildKite thing that only runs on oven-sh/bun that submits a PR updating this file? Or maybe we only store it as an artifact in a recurring BuildKite job itself? |
Replaces index % maxShards with longest-processing-time-first assignment driven by test/expected-durations.json, a per-file median wall-clock table with separate default and asan columns. Every shard deterministically computes the same assignment and picks its own bin; unknown files fall back to the table median so new tests still spread evenly. On linux-x64-debian-13 timings, 10 shards go from a 497s longest / 159s shortest round-robin spread to 315s / 315s with LPT (asan: 1294s..877s -> 1062s flat). scripts/update-test-durations.mjs regenerates the table from the last N complete Buildkite runs of the release + asan linux-x64 lanes; the checked-in file was generated from builds 69524/69519/69515. .buildkite/update-test-durations.yml is a standalone pipeline for a weekly schedule that reruns the script and uploads the result as an artifact.
|
Done in cccae2f:
Simulated balance on the current test set:
I went with artifact-upload for the recurring job rather than auto-PR; the runner reads the checked-in copy, so the artifact is for humans to pull when rebalancing, not a runtime dependency. Happy to switch to auto-PR if you want it fully hands-off, but that needs a bot token with push rights wired into the scheduled pipeline. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/runner.node.mjs`:
- Around line 589-595: `limitFor` in `scripts/runner.node.mjs` should honor the
global `--parallel` limiter before any test-specific limiter selection. Update
the logic around `parallelSafeLimit`/`limit` so that when `parallelism` is
enabled, both parallel-safe and non-parallel-safe tests route through the same
global width, preserving the “supersedes this” contract. Keep
`isParallelSafeTest` and `limitFor` as the main entry points, but make the
limiter choice depend on the active parallel mode rather than always allowing
`parallelSafeLimit`.
In `@scripts/update-test-durations.mjs`:
- Around line 108-130: The collect() worker currently lets a single failed api()
call for a job log abort the entire update-test-durations run, losing all
previously gathered samples. Update the worker inside collect() to catch errors
per job when fetching builds/${build}/jobs/${j.id}/log.txt, log or skip
transient failures like 429/5xx, and continue processing the remaining jobs
instead of letting Promise.all fail the whole batch.
- Around line 46-52: The Buildkite request helper api() currently uses fetch
without any timeout or AbortSignal, so a stalled request can hang the scheduled
job indefinitely. Update the api() helper to create and pass an AbortSignal with
a reasonable timeout, and ensure the timeout is cleared/handled around the
existing fetch and response error handling so requests fail fast instead of
waiting forever.
- Around line 93-106: The source build selection in findSourceBuilds() is
pulling builds from every branch, which can skew the sampled durations. Update
the builds API request in that function to filter to the default branch (for
example, add branch=main or the pipeline’s default branch) so only
representative builds are considered before collecting picked build numbers.
- Around line 58-82: parseLog currently hardcodes the group-header and
end-marker formats from startGroup(), so it can silently return wrong durations
if scripts/utils.mjs::startGroup() or scripts/runner.node.mjs changes. Tighten
this contract by either sharing the same formatter/parser helper between
parseLog and startGroup, or by adding explicit validation in parseLog for the
expected marker shape before computing durations. Keep the fix centered around
parseLog and the startGroup output format so changes fail clearly instead of
producing bad data.
In `@test/napi/node-napi-tests/harness.ts`:
- Around line 134-141: The direct compile fallback path in harness.ts waits on
child.exited before reading child.stderr, which can deadlock if the stderr pipe
fills. Update the direct compile logic and buildWithNodeGyp to start draining
stderr in parallel with the child process execution, then await both the exit
and the stderr read before deciding whether to fall back. Use the child,
child.exited, and child.stderr handling in these functions as the main places to
fix.
🪄 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: 52860a36-0eda-45a4-8275-af547c3a347d
📒 Files selected for processing (7)
.buildkite/update-test-durations.yml.gitattributes.prettierignorescripts/runner.node.mjsscripts/update-test-durations.mjstest/expected-durations.jsontest/napi/node-napi-tests/harness.ts
…derr concurrently, harden duration generator - runner: when --parallel is set, route parallel-safe tests through the same pLimit instance as everything else instead of a second limiter of the same width (two limiters at width N can admit 2N processes). - napi harness: drain stderr concurrently with child.exited in both tryBuildFast and buildWithNodeGyp so a verbose failing compile cannot fill the pipe buffer and wedge. - update-test-durations: 60s AbortSignal timeout on API requests; skip a single shard log on transient fetch failure instead of aborting the whole run; fail loudly if <1000 paths are parsed so a log-format change cannot silently produce an empty table. Not filtering to branch=main because main builds skip the test-bun steps (documented in-source).
4-wide surfaced new retry-flakes on the Windows lanes in tight-timeout and stdin-write node/test/parallel tests (test-repl-close EPIPE, test-https-proxy-request*, test-https-timeout) that did not flake on seven recent baseline builds. Process creation is heavier on Windows and the shards there are fewer (8 vs 20), so 4 concurrent bun processes contend enough to push those races over. Posix stays at 4.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/runner.node.mjs (1)
2121-2135: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRead
test/expected-durations.jsonhere.cwdresolves to the repo root, sojoin(cwd, "expected-durations.json")misses the file written byscripts/update-test-durations.mjsand falls back to index sharding, undoing the LPT balancing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/runner.node.mjs` around lines 2121 - 2135, The expected durations file is being read from the wrong location in the sharding setup, so the loader in runner.node.mjs falls back to index-based sharding instead of LPT balancing. Update the JSON read in the duration-loading block to use the test/expected-durations.json path that matches scripts/update-test-durations.mjs, and keep the rest of the duration parsing logic in the same section around costOf and the durations map unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/runner.node.mjs`:
- Around line 2121-2135: The expected durations file is being read from the
wrong location in the sharding setup, so the loader in runner.node.mjs falls
back to index-based sharding instead of LPT balancing. Update the JSON read in
the duration-loading block to use the test/expected-durations.json path that
matches scripts/update-test-durations.mjs, and keep the rest of the duration
parsing logic in the same section around costOf and the durations map unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17e0e4d6-b919-4ff4-8412-3a870e26527e
📒 Files selected for processing (3)
scripts/runner.node.mjsscripts/update-test-durations.mjstest/napi/node-napi-tests/harness.ts
The eight backend:"cli" cases each spawn a full bun build --compile link (hundreds of MB); running them all at once drove the linux lanes to SIGTERM at ~65s on six variants. expectBundled already forces backend:"api" to it.serial, so describe.concurrent only overlapped the CLI cases.
|
@robobun looks like there are newly flaky tests to address + windows has more test time variance (should windows gets its own section separate from default and Asan)? |
…ndows durations column
runner: split test execution into two non-overlapping phases. Phase 1
runs every non-parallel-safe test through the serial limiter; phase 2
then runs js/{node,bun}/test/parallel/ through the N-wide limiter. The
previous two-independent-limiters design let up to N parallel-safe
processes run alongside a serial test, which surfaced new retry-flakes
in tight-timeout and port-bind tests (dns-tcp-bidirectional-poll,
test-https-timeout, test-repl-close) that did not flake on 10 baseline
builds.
bun-upgrade.test.ts: revert describe.concurrent. Concurrent
copyFile(bunExe(), execPath) + spawn(execPath) races to ETXTBSY on linux.
bun-install-proxy.test.ts: revert to the dedicated docker run on
hardcoded :3128. The shared ensure("squid") path flaked ConnectionClosed
4x in one build vs 2x across 10 baseline builds; dropping the prestart-map
entry too.
zlib.test.js brotli streaming: set BROTLI_PARAM_QUALITY 4 so 8 MB of
random input completes inside the 15s budget on a contended runner; the
assertion is quality-independent.
expected-durations: add a windows column (windows-x64-2019 lane). Process
spawn is ~2x slower than linux there and many tests skipIf(isWindows), so
packing with the linux timings left 150-300s of spread across the 8
Windows shards. runner.node.mjs picks the windows column when the step is
a windows lane. Regenerated from builds 69636/69628/69627.
|
Both addressed in 584bb9f: Newly flaky tests (compared against 10 baseline builds):
Everything else in #69770's flaky list ( Windows durations column: yes, added. For reference, #69770 measured wall clock (20 shards linux, 8 shards windows) vs baseline #69493:
|
|
Heads up: the N-wide phase this introduced for |
|
Since the tests in |
What does this PR do?
Speeds up the test suite across ~60 files without changing what is being asserted, by:
test.concurrent/describe.concurrentwhere tests spawn independent subprocesses or do I/O with no shared mutable state (bundler, upgrade, workspaces, multi-run, no-orphans, spawn, websocket, serve-epoll, structured-clone, proxy-stress, FormData, grpc-js idle-timer, password, type-export, and more).test/napi/node-napi-tests/harness.tsnow compiles the trivialbinding.gypshape directly with clang when the node-headers cache is present, falling back tonode-gyp configure buildotherwise.napi.test.ts/napi-value-ffi/napi-finalizer-delete-refskipbun installwhen the built.nodeis already fresh. The isolate-finalizer addon moves to its own fixture directory so30205.test.tsno longer rebuilds all of napi-app.test/docker/prestart-map.mjs(consumable from plain Node) andscripts/runner.node.mjssorts docker-backed test files toward the end of each shard so container cold start overlaps with the non-docker tests that run first.test/docker/coordinator.tscaches the last healthyServiceInfoper service and re-validates it with a cheap TCP probe instead of paying the fullcompose up --waitpath on every request.PUPPETEER_SKIP_DOWNLOADand builds both variants concurrently,test-performance-eventloopdelayspins for 100ms instead of 1s.How did you verify your code works?
CI. On
linux-x64-asan-debian-13(build #69715 vs the median of 5 recent PR builds), the 60 comparable changed files go from 1303s to 1103s total (-15.3%); the longest shard on that lane was 692s vs 718-932s across the baseline builds. Largest per-file wins:websocket-syscall-fault-38s,FormData-29s,proxy-stress-concurrent-25s,node/net/handle-leak-25s,zlib/leak-20s,fetch-backpressure-20s.A few files in the first run report slower because all 60+ touched files are sorted to the very front of their shard (position 3-5/~227), where they contend with the shard's
bun installstep and cold docker-compose. Those are measurement artifacts of touching the file, not of the change itself; the same files run at normal positions once this merges.Three changes from the original commit were reverted in e19cfca after failing in CI:
bun-security-scanner-matrix-runner.ts:toMatchSnapshot()is rejected insidetest.concurrent(1440+ snapshot entries here cannot be made inline).test/js/node/test/parallel/test-dns-setserver-when-querying.jsandtest-stream-iter-broadcast-from.js: restored to upstream per theparallel/convention; the dns rewrite also exposed a null deref indns.rs remove_timeron ASAN that is out of scope here.