test: run the bytecode portability builds concurrently and share them between tests - #40898
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 22 days. After that, they cost $0.25 per reviewed file. Or wait 45 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 4:18 AM PT - Aug 29th, 2026
❌ @robobun, your commit 6ae2b14 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40898That installs a local version of the PR into your bun-40898 --bun |
|
Status: test-only change, ready for review. Verified locally with CI (build #108308), per-file time of this test, 22 pass on every lane:
The build is red for a reason outside this diff: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup that shares bundler builds across tests without weakening any assertion.
What was reviewed:
- Snapshot ordering:
outputsinsertion order changed (vm.Script entries now populated before bundler builds), but the inline snapshot serializer sorts keys alphabetically, so the existing snapshot still matches. - Shared
jscbuffer in the reject tests:spoil(Buffer.from(jsc))copies before mutating, so the three concurrent variants don't clobber each other's cached bytes. - Memoization:
build()keys onname(unique acrossbundlerBuilds) and readsbuilds.sizesynchronously beforeset, so per-build outdirs never collide even undertest.concurrent. - The
--compilestderr check moves fromnot.toContain("error")totoBe("")— a strengthening, matching whatbundle()already asserts.
Extended reasoning...
Overview
This PR refactors a single test file, test/bundler/bundler_bytecode_portable.test.ts, to cut its wall time roughly in half by (a) launching all bun build --bytecode child processes concurrently and doing the in-process vm.Script/SourceTextModule encoding while they run, and (b) memoizing each corpus build in a file-lifetime tempDir so the snapshot test, the per-entry load tests, the determinism test's reference build, and the .jsc rejection tests all share one build per entry instead of re-bundling. bundle() now labels its assertions with the invoked command and returns the output path; the --compile test's local build variable is renamed to compile to avoid shadowing the new helper. No production code is touched.
Security risks
None. The change is confined to test orchestration: it rearranges when child processes are spawned and where their outputs land on disk, and adds assertion labels. It introduces no new inputs, no network access, no credential handling, and no changes to what is being asserted (other than tightening one stderr check).
Level of scrutiny
Moderate — the file pins JSC bytecode fingerprints across platforms, so the main risk of a refactor here is silently weakening coverage or making the snapshot pass for the wrong reason. I checked each of those failure modes: the inline snapshot is byte-identical (key order is serializer-sorted, so the changed outputs insertion order is irrelevant); dumpPayloads() still sees every payload because fingerprint() is still called on every entry; the memoized build() uses name as its key and every bundlerBuilds entry has a distinct name; each build gets its own numbered subdirectory under buildsDir so readdirSync(outdir) still sees exactly two files; the reject tests write the shared js alongside the spoiled jsc into a fresh temp dir and copy the buffer before mutating it. The determinism test's reference now reuses the memoized default-env build of bundlerBuilds[0], which is semantically identical to the fresh bundle() it replaced. The synchronous vm encoding block means the un-awaited Promise.all cannot reject before a handler is attached, so there is no unhandled-rejection window.
Other factors
The change follows the repo's test conventions: tempDir from harness with explicit afterAll disposal for the file-lifetime dir, await using on spawned processes, stderr asserted before exit code, test.concurrent for independent subprocess tests, and {...bunEnv, ...} spreads. No CODEOWNERS entry covers this path. The PR description reports three passing bun bd test runs (22 pass each) and release-binary timings, and the bug-hunting pass exited on a dry streak with no findings.
3 participants
Problem
test/bundler/bundler_bytecode_portable.test.tstakes 34s on the x64-asan lane (build #108217). Locally: 342s with the debug build, 5.6s with the release binary.bun build --bytecodeprocesses one after another. The load and reject tests then bundled the same entries again. The libraries.js build alone takes 88s in the debug build, and the file ran it twice.Fix
build()memoizes one build per entry in a file-level temp dir. The snapshot, load and reject tests share it: 24 spawned builds per run instead of 37. A test run alone with-tstill starts what it needs.bundle()names the command in its stderr, exit code and output listing checks, so a failed build fails with the build error, not a fingerprint mismatch. The--compiletests require empty stderr, notnot.toContain("error").bun bd test test/bundler/bundler_bytecode_portable.test.ts341.6s before, 133 to 138s after (3 runs, 22 pass). Release binary: 5.6s before, 2.8s after.Background
bun build --bytecodeis a child process. The in-process encodes are synchronous JS: they cannot overlap each other, but they can overlap child processes.Notes
Timing of the unchanged file, debug build with
--timeout=270000(the CI asan value): 341.6s total. Snapshot test 189.2s, process test 41.1s, concurrent group about 111s, of which the libraries.js load test took 107.6s (88s bundle plus 19s run). The 5s default timeout had to be raised: with it, the concurrent tests time out locally.Per-build times, debug, sequential (147s total): libraries.js 88s, svelte 11.6s, happy-dom 11s, all.js 9.2s,
--minifyall.js 8.7s, react-dom 5.7s, the rest under 3s. The same 16 builds withPromise.all: 92s wall, that is the libraries.js build. For libraries.js, bundling without--bytecodetakes 7s, so the rest is bytecode generation.In-process encodes, debug: vm.Script typescript.js 37s, the internal modules loop 18s, the others under 1.3s. Everything in-process is about 41s and is now hidden behind the 92s of builds.
After the change, debug: snapshot test 95s, process test 20.5s, concurrent group about 20s (libraries.js load test 19.6s, run only). Three full runs: 136.4s, 133.4s, 133.8s, 22 pass each. Release binary: snapshot test 3.0s to 2.0s, process test 0.6s to 0.3s, libraries.js load test 1.9s to 0.4s.
Memory: peak RSS per build process is 30 to 90 MB with the release binary (libraries.js 551 MB, sum of all 16 about 1.2 GB) and about 340 MB under ASAN (libraries.js 1.0 GB, sum 5.7 GB). The x64 release test lane has 8 GB, the asan lanes 64 GB.
Redundant entries: none. All 16
jshashes and all 26 fingerprints in the snapshot are distinct, so no entry was removed. The redundant work was the same entry bundled by several tests.Failure modes, probed on purpose. A missing entry fails the snapshot test and the matching load test with: error:
`bun build --bytecode ./missing.js` wrote to stderrfollowed by the bundler'sModuleNotFoundline. A changed fingerprint fails with the existing snapshot diff, which shows the entry key as context and both sha256 values, so no second message was added for it.dumpPayloads()still prints every payload on a mismatch.bun buildignores unknown flags such as--bogus-flag(exit 0). That is unrelated and not changed here.tsc -p test/tsconfig.jsonreports the same two pre-existingcreateCachedDataerrors on this file as on main.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_bytecode_portable.test.ts