Conversation
Two compile tests wrote their output executable into the test runner's cwd (the repo root under bun test and in CI) and never removed it: - bun-build-compile.test.ts "compile with embedded resources" passed a bare compile.outfile with no outdir, which Bun.build resolves against the process cwd, leaving ./app-with-resources behind (about 1 GB with a debug build). - standalone.test.ts "CLI --compile --target=browser with non-HTML" spawned bun build --compile without --outfile and without cwd, so the CLI's default output name landed in the runner's cwd as ./app. Point both at their tempDir and assert the output path, so a bare name fails the test instead of silently leaving a binary in cwd.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe embedded-resources compile test now uses an absolute temporary output path and verifies the expected platform-specific executable path. ChangesBundler test validation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This test-only change keeps compiled executables in temporary directories and verifies their exact output path, preventing stray files in the repository; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced by running the two tests against the current release build from the repo root ( 51914ed folds in the one point from review (assert stderr together with the exit code in the standalone fallback test). No open review threads. Waiting on CI. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only hermeticity fix that keeps compiled executables inside their temp dirs.
What was reviewed:
- Verified the new absolute
outfileinbun-build-compile.test.tsmatches the pattern every other test in that file already uses, and that the tightenedtoBeassertion is safe (with nooutdir, the artifact path is the passed outfile verbatim +.exeon Windows). - Verified the added
cwd: String(dir)andexistsSynccheck instandalone.test.tsfollow existing conventions in that file (forward-slash paths withexistsSyncare already used elsewhere and work on Windows). - Confirmed no runtime code is touched and the new assertions are strictly stronger than what they replace.
Extended reasoning...
Overview
This PR touches two test files only: test/bundler/bun-build-compile.test.ts and test/bundler/standalone.test.ts. Both changes fix test hermeticity — the tests were writing ~75MB (or ~1GB debug) compiled executables into the test runner's cwd (the repo root) and never cleaning them up. The fix redirects each output into the test's temp dir, which using dir = tempDir(...) already cleans up.
In bun-build-compile.test.ts, the bare outfile: "app-with-resources" becomes outfile: join(dir, "app-with-resources"), and the assertion tightens from toEndWith(...) to toBe(...) on the exact absolute path. This is exactly what every neighbouring test in the file does (e.g., "compile with relative outfile paths", "output binary has valid executable header", the ELF section tests).
In standalone.test.ts, the spawn gains cwd: String(dir) so the CLI's default ./app output lands in the temp dir, plus an existsSync assertion that the fallback actually produced an executable (previously only exitCode was checked). The isWindows import is added to handle the .exe suffix.
Security risks
None. Test-only changes with no runtime code modification, no new external dependencies, no network access, no changes to how untrusted input is handled.
Level of scrutiny
Low. This is a mechanical test hygiene fix following established patterns already present in both files. The changes are small, self-contained, and the intent (stop polluting the repo checkout) is unambiguous. The only subtle point is whether the toBe assertion on the output path is correct across platforms — the PR description explicitly addresses this ("with no outdir the artifact path is the outfile verbatim"), and the author verified the tests pass on the debug build.
Other factors
- No prior human or bot reviews to address (only a coderabbit rate-limit notice).
- The PR description includes verification that both test files pass with
bun bd testand leavegit statusclean, and that the new assertions fail when reverted to the old bare-name variants. - The added
existsSyncassertion instandalone.test.tsstrengthens the test (it now verifies an executable was actually produced, not just that the process exited 0), which is a net improvement per the repo's review guidelines on assertion strength. - Forward-slash path in the Windows
existsSynccheck is consistent with existing usage in the same file (e.g., line ~280existsSync(\${outdir}/index.html`)`) and Node's fs handles it.
### What
`test/bundler/bun-build-compile.test.ts` "compile with relative outfile
paths" ran three `Bun.build({ compile: { outfile } })` calls inside one
test. Every compile copies and rewrites the whole bun binary, which is
about 1 GB with a debug+ASAN build and takes roughly 2.5s there (every
single-compile test in the file reports 2.4s to 2.9s), so the test
needed 7s or more of a 5s default per-test budget. On the unmodified
file, `bun bd test test/bundler/bun-build-compile.test.ts` at
9fcdea8:
```
(fail) Bun.build compile > compile with relative outfile paths [5523.70ms]
^ this test timed out after 5000ms.
(pass) Bun.build compile > compile with embedded resources uses correct module prefix [5227.91ms]
```
The failure is intermittent (the session that reported it saw it in
about 1 run in 3, with the passing runs taking 7.0s to 7.2s). The
compile step runs on the JS thread inside the bundle completion task
(`do_compilation` is called from `JSBundleCompletionTask::on_complete`),
so the timeout can only be observed between compiles: when the first two
compiles add up to more than 5s the test fails after the second one,
otherwise the third compile starts and the test finishes late but
passes. The third compile of a timed-out run also keeps going and slows
down the next test, which is why "embedded resources" shows 5.2s above
and 2.5s once this is fixed.
This only affects the 5s default used by a plain `bun test` / `bun bd
test` run; the CI runner passes a much larger `--timeout`, and with a
release binary each compile takes about 0.13s.
### Fix
Turn the three cases into a `test.each`, so every test does exactly one
compile, the same shape as the rest of the file. The per-test timeout is
left at the default rather than raised: the workload per test is what
was out of proportion.
While rewriting the assertion, the three `toContain` / `toEndWith`
checks on the reported path become an exact comparison against the
outfile that was passed in (`.exe` appended on Windows; with no `outdir`
the artifact path is the outfile verbatim), and each case also checks
that the executable exists at that path, which is what the nested
`output/nested/` and `a/b/c/d/` cases are there to prove. Nothing else
in the file changes.
### Verification
`bun bd test test/bundler/bun-build-compile.test.ts` (debug+ASAN), two
runs, 14 pass / 1 skip each, about 40s per run:
```
(pass) Bun.build compile > compile writes the executable to outfile output/nested/app1 [2567.03ms]
(pass) Bun.build compile > compile writes the executable to outfile app2 [2576.66ms]
(pass) Bun.build compile > compile writes the executable to outfile a/b/c/d/app3 [2590.81ms]
(pass) Bun.build compile > compile with embedded resources uses correct module prefix [2501.16ms]
```
The same file also passes with a release bun on Linux
(`USE_SYSTEM_BUN=1`) and on Windows Server 2019 x64 (release bun 1.4.0,
where the exact path assertion exercises the `.exe` branch).
Related: #37389 raises this test's timeout to 60s as a side change of an
unrelated fix; whichever lands second drops or rebases a one-line hunk.
#37397 fixes the neighbouring "embedded resources" test in the same file
and does not overlap with this hunk.
…ack test A failing compile now reports its diagnostics instead of only the exit code.
|
Updated 7:14 AM PT - Aug 29th, 2026
❌ @robobun, your commit 7c4394b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37397That installs a local version of the PR into your bun-37397 --bun |
There was a problem hiding this comment.
LGTM — test-only hygiene fix that stops two compile tests from dropping ~1 GB executables into the repo root.
What was reviewed:
- Both changes match the neighbouring tests' patterns (
join(dir, ...)for outfile,cwd: String(dir)for spawns) already used elsewhere in the same files. - Assertions were strengthened, not weakened:
toBe(outfile)catches a regression back to a bare name, and the standalone test now checks stderr and that the executable actually exists. - Checked that the new
stderr: ""assertion matches what the sibling CLI test instandalone.test.tsalready asserts forbun build --compile.
Extended reasoning...
Overview
Test-only change touching two bundler test files. In bun-build-compile.test.ts, the "embedded resources" test's compile.outfile is changed from a bare "app-with-resources" (which Bun.build resolves against process cwd) to join(dir, "app-with-resources"), and the path assertion is tightened from toEndWith to toBe. In standalone.test.ts, the CLI fallback test gains cwd: String(dir) so the default --outfile-less compile writes into the temp dir, and the assertion is upgraded to a combined { stderr, exitCode, executable } object that also verifies the executable was actually produced.
Security risks
None. No runtime code is touched; this only redirects where two tests write their compiled artifacts and adds assertions.
Level of scrutiny
Low. This is a hermeticity fix for tests that were leaking large binaries into the checkout on persistent CI runners and developer machines. The changes copy patterns already used by every other test in the same files (absolute outfile via join(dir, ...), spawn with cwd: String(dir)). No production code paths change.
Other factors
- Assertions get strictly stronger:
toBe(outfile)now fails loudly if someone reverts to a bare outfile, and the standalone test now checks the fallback actually produced an executable rather than onlyexitCode === 0. This aligns with the repo's "strongest invariant" test guideline. - The new
stderr: ""assertion mirrors the existing "CLI --compile --target=browser produces single file" test in the same file, so it is known-safe forbun build --compilesuccess output. - The one Windows-specific concern (does
outputs[0].pathbyte-matchjoin()'s backslash path when nooutdiris set) is addressed in the PR description — with nooutdirthe artifact path is the outfile string verbatim, so.exesuffixing is the only transform. The author reports both files passing underbun bd test. - No prior reviewer comments to address; the bug hunting system found nothing.
Resolve test/bundler/standalone.test.ts with the version on main. #40730 already made that test spawn the CLI with cwd set to its temp dir and assert that the executable exists there. Only the bun-build-compile.test.ts change remains.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
The diff is a 4-line test fix in Two CI runs failed only on tests this diff does not touch (http2, URL/IDNA, install registry races, TLS on darwin). The failures reproduce on main and are reported separately. The change is ready for review. |
There was a problem hiding this comment.
LGTM — after the merge with main, this is down to a 4-line test-hygiene fix in one file.
What was reviewed:
outfilenow built viajoin(dir + "", ...), matching the neighbouring "output binary has valid executable header" test in the same file.- The tightened
toBe(isWindows ? \${outfile}.exe` : outfile)assertion —Bun.buildreturns the outfile verbatim (plus.exeon Windows) when nooutdir` is set, so exact equality holds. - The
standalone.test.tshalf of the original PR was dropped in the merge because #40730 already landed the equivalent fix on main; net diff no longer touches that file.
Extended reasoning...
Overview
The PR originally fixed two compile tests that dropped executables into the test runner's cwd. Since the last review, a merge with main (5c4cf85) resolved test/bundler/standalone.test.ts to main's version because #40730 had independently landed the same fix there. The remaining net diff is four effective lines in test/bundler/bun-build-compile.test.ts: build outfile as an absolute path inside the tempDir, swap the toEndWith suffix check for an exact toBe (with the .exe suffix on Windows), and add a one-line comment explaining why a relative outfile is wrong. The pattern is copied verbatim from the test two blocks below in the same file (const outfile = join(dir + "", "app-out")).
Security risks
None. Test-only change; no runtime, native, or bindings code is touched. No new external inputs, no network, no filesystem operations beyond redirecting an existing artifact into an already-managed temp directory.
Level of scrutiny
Low. This is a mechanical test-hygiene correction that follows the file's established convention exactly. The Windows .exe branch uses the harness isWindows flag as required. The tightened assertion is strictly stronger than the old one and would catch a regression back to the bare-name form. using dir = tempDir(...) already handles cleanup, so the moved artifact is now removed with the rest of the temp directory.
Other factors
No CODEOWNERS entry covers test/bundler/. The bug-hunt exit reason was dry_streak with no findings and no ruled-out candidates. No third-party CHANGES_REQUESTED reviews are outstanding in the timeline. The scope reduction since the prior review is the substantive change prompting this approval.
This branch has not been deployed
1 participant
Problem
bun test test/bundler/bun-build-compile.test.tsleaves an untrackedapp-with-resourcesexecutable in the repo root. It is a copy of the bun binary: 74 MB with a release build, about 800 MB with a debug build. Nothing removes it, and the name is not gitignored.test/bundler/bun-build-compile.test.ts:287on main). It passescompile.outfile: "app-with-resources"with nooutdir.Bun.buildresolves a relativecompile.outfileagainst the process cwd, so the executable lands in the test runner's cwd instead of the test's temp dir. ThetoEndWithassertion only checks the suffix and does not notice.Fix
join(dir + "", "app-with-resources")as the outfile, the same as the neighbouring tests in the file.using dir = tempDir(...)then removes the executable with the temp dir.outputs[0].pathis exactly that path (.exeappended on Windows). With nooutdirthe artifact path is the outfile verbatim, so a bare name now fails withReceived: "/workspace/bun/app-with-resources"instead of leaving a file behind.bun bd test test/bundler/bun-build-compile.test.ts. The fixed test passes andgit statusis clean afterwards. The same command on main leaves the 817 MBapp-with-resourcesin the repo root.Background
bunfig.tomlsetsroot = "test"and CI spawnsbun testfrom the checkout, so the test runner's cwd is the repo root.test/bundler/standalone.test.ts("CLI --compile --target=browser with non-HTML"), which wrote./appinto the cwd. Bun.build: write the default compile outfile to the working directory #40730 landed an equivalent change for that test on main. The merge of main into this branch keeps main's version, so only thebun-build-compile.test.tschange remains.Notes
Original reproduction, from the repo root with the release build:
The other compile tests under
test/were checked at the time the PR was opened:itBundledcompiles to an absolute outfile under its own temp root, and the remaining directBun.buildandbun build --compilecalls pass an absolute outfile, setoutdirto the temp dir, or spawn withcwdset to it.compile-windows-metadata.test.ts"relative outdir with compile" builds into<cwd>/out/on Windows on purpose, removes the executable afterwards, andoutis gitignored.Under the debug build in a capped container, three unrelated bytecode compile tests in the same file hit their 5 s and 60 s timeouts. They are unchanged from main here and passed on every lane in the earlier CI run of this PR.
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-compile.test.ts