Make the bundler tests use the API by default in most cases - #22646
Conversation
|
Updated 2:06 AM PT - Sep 14th, 2025
❌ @Jarred-Sumner, your commit f7b22b8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 22646That installs a local version of the PR into your bun-22646 --bun |
WalkthroughAdded errdefer cleanup for ArrayList instances in the Zig JSON parser. Updated test harness to change default backend resolution and run Bun.build with a temporary CWD. Multiple bundler tests now explicitly set backend: "cli". Changes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (4)
test/bundler/compile-argv.test.ts (2)
10-10: Pinning these tests to the CLI backend is the right call.This locks the intended compile/execArgv semantics regardless of harness default-backend heuristics. Consider adding a short inline comment so future readers don’t “simplify” it away.
Apply this small annotation for clarity:
- backend: "cli", + // Force CLI backend: validate compile execArgv vs argv separation using the CLI bundler. + backend: "cli",Also applies to: 56-56, 120-120
86-88: Make argv[0] assertion OS‑agnostic to avoid flakiness.These assertions hardcode argv[0] === "bun", which may differ (e.g., bun.exe on Windows). Prefer a suffix or regex match.
- if (process.argv[0] !== "bun") { + if (!/bun(\.exe)?$/i.test(process.argv[0])) { console.error("FAIL: Expected argv[0] to be 'bun', got", process.argv[0]); process.exit(1); }Please run this file on Windows in CI to confirm no regressions in argv semantics for the CLI backend.
Also applies to: 144-146
test/bundler/bundler_compile.test.ts (1)
109-109: Explicitly using the CLI backend for these compile cases is appropriate.These scenarios (worker relative paths, bytecode, and import.meta.main inlining) are sensitive to backend differences; pinning avoids accidental drift if harness defaults change.
Add a brief rationale to each to prevent future “cleanup”:
- itBundled("compile/WorkerRelativePathNoExtension", { - backend: "cli", + itBundled("compile/WorkerRelativePathNoExtension", { + // Force CLI backend: validate worker path resolution under compile. + backend: "cli",- itBundled("compile/WorkerRelativePathTSExtension", { - backend: "cli", + itBundled("compile/WorkerRelativePathTSExtension", { + // Force CLI backend: validate worker .ts extension handling under compile. + backend: "cli",- itBundled("compile/WorkerRelativePathTSExtensionBytecode", { - backend: "cli", + itBundled("compile/WorkerRelativePathTSExtensionBytecode", { + // Force CLI backend: bytecode + worker path resolution via CLI bundler. + backend: "cli",- itBundled("compile/ImportMetaMain", { - compile: true, - backend: "cli", + itBundled("compile/ImportMetaMain", { + compile: true, + // Force CLI backend: ensures import.meta.main and require.main semantics from CLI compile. + backend: "cli",Also applies to: 129-129, 148-148, 564-564
test/bundler/bundler_footer.test.ts (1)
7-7: Good: footers validated under the CLI backend.Footer emission can vary by backend; pinning to CLI removes ambiguity. Consider adding a short comment to codify the intent.
- backend: "cli", + // Force CLI backend: verify footer is appended by CLI bundler. + backend: "cli",Also applies to: 20-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/interchange/json.zig(2 hunks)test/bundler/bundler_compile.test.ts(4 hunks)test/bundler/bundler_edgecase.test.ts(2 hunks)test/bundler/bundler_footer.test.ts(2 hunks)test/bundler/bundler_regressions.test.ts(1 hunks)test/bundler/compile-argv.test.ts(3 hunks)test/bundler/esbuild/dce.test.ts(1 hunks)test/bundler/expectBundled.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/interchange/json.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/interchange/json.zig
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: For platform-specific changes, run bun run zig:check-all to compile on all platforms
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.tstest/bundler/esbuild/dce.test.tstest/bundler/bundler_footer.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/compile-argv.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/bundler/**/* : Place bundler/transpiler/CSS/bun build tests under test/bundler/
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/expectBundled.tstest/bundler/bundler_regressions.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/bundler/bundler_compile.test.ts
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Follow the build pipeline: Source TS/JS → Preprocessor → Bundler → C++ Headers; IDs assigned A–Z; `$` replaced with `__intrinsic__`; `require("x")` replaced with `$requireId(n)`; `export default` converted to `return`; `__intrinsic__` replaced with `@`; inlined into C++; modules loaded by numeric ID
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/bundler/bundler_compile.test.tstest/bundler/expectBundled.tstest/bundler/esbuild/dce.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : Place CLI command tests (e.g., bun install, bun init) under test/cli/
Applied to files:
test/bundler/compile-argv.test.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Applied to files:
src/interchange/json.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Applied to files:
src/interchange/json.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/interchange/json.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Author modules as CommonJS-style with `require(...)` and export via `export default {}` (no ESM `import`/named exports)
Applied to files:
test/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/bundler/expectBundled.ts
🔇 Additional comments (7)
test/bundler/bundler_regressions.test.ts (1)
219-219: Explicitly pinning backend to CLI is appropriate here.This test uses entryPointsRaw and expects CLI-specific behavior; forcing backend: "cli" avoids accidental API execution as the harness defaults evolve.
Please confirm no other tests relying on entryPointsRaw remain without backend: "cli".
src/interchange/json.zig (2)
207-208: Good: errdefer for properties mirrors array handling.Matches the array case and prevents leaks when object parsing errors early.
173-174: Approve — errdefer correctly frees exprs on error; run Zig checks locally
errdefer ensures the exprs ArrayList storage is freed on error paths; moveFromList transfers ownership so the happy path is unaffected.
Location: src/interchange/json.zig (lines 173–174)
Run locally: bun run zig:check-all (sandbox couldn't execute this: bun not found / nl unavailable).test/bundler/bundler_edgecase.test.ts (2)
277-277: Good: force CLI for invalid loader case.The asserted error string is CLI-specific; pinning backend avoids false negatives when API is chosen by default.
2075-2075: Good: force CLI for multi-output + outfile error.The harness would otherwise preempt with its own validation; this ensures the test checks Bun CLI’s error message.
test/bundler/esbuild/dce.test.ts (1)
1245-1245: Stabilize DCE annotation tests by pinning CLI.Ensures consistent semantics across variants (minify/emitDCEAnnotations) regardless of default backend heuristics.
test/bundler/expectBundled.ts (1)
575-591: ```shell
#!/bin/bash
set -euo pipefailFILE="test/bundler/expectBundled.ts"
echo "=== Checking repository for ${FILE} ==="
if [ ! -f "$FILE" ]; then
echo "FILE_NOT_FOUND"
exit 0
fiecho
echo "=== File excerpt (lines 540-610) ==="
awk 'NR>=540 && NR<=610{printf("%6d %s\n", NR, $0)}' "$FILE" || trueecho
echo "=== Search: 'backend =' occurrences ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore "backend\s*=" || true
else
grep -RIn --exclude-dir=.git --exclude-dir=node_modules -E "backend\s*=" || true
fiecho
echo "=== Search: backend declarations (let|var|const backend) ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore "\b(let|var|const)\s+backend\b" || true
else
grep -RIn --exclude-dir=.git --exclude-dir=node_modules -E "\b(let|var|const)\s+backend\b" || true
fiecho
echo "=== Search: chooseBundlerBackend (collision check) ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore "chooseBundlerBackend" || true
else
grep -RIn --exclude-dir=.git --exclude-dir=node_modules -e "chooseBundlerBackend" || true
fiecho
echo "=== Occurrences of variables used in the expression ==="
for v in dotenv jsx production bundling run target emitDCEAnnotations bundleWarnings env define; do
echo
echo "---- $v ----"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore "\b$v\b" || true
else
grep -RIn --exclude-dir=.git --exclude-dir=node_modules -E "\b$v\b" || true
fi
doneecho
echo "=== Done ==="</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| const cwd = process.cwd(); | ||
| process.chdir(root); | ||
| try { | ||
| build = await Bun.build(buildConfig); | ||
| } finally { | ||
| process.chdir(cwd); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential flakiness: global process.chdir around Bun.build is not concurrency‑safe.
If tests run concurrently in the same process, chdir() can race and misdirect builds. Serialize these sections or avoid chdir entirely.
Apply this diff to use a simple async mutex around cwd switches:
- const cwd = process.cwd();
- process.chdir(root);
- try {
- build = await Bun.build(buildConfig);
- } finally {
- process.chdir(cwd);
- }
+ build = await withCwd(root, () => Bun.build(buildConfig));Add this helper once (outside the hunk, top-level of this file):
let __cwdQueue: Promise<void> = Promise.resolve();
async function withCwd<T>(dir: string, fn: () => Promise<T> | T): Promise<T> {
let release!: () => void;
const next = new Promise<void>(r => (release = r));
const prev = __cwdQueue;
__cwdQueue = next;
await prev;
const old = process.cwd();
process.chdir(dir);
try {
return await fn();
} finally {
process.chdir(old);
release();
}
}Alternatively, remove the need to chdir by ensuring all API entrypoints/paths are absolute for the API path.
Please run the bundler suite with parallelism enabled to smoke-test races:
🏁 Script executed:
#!/bin/bash
# from repo root
bun test test/bundler -j $(nproc)Length of output: 85
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Checking for ripgrep (rg)..."
if command -v rg >/dev/null 2>&1; then
echo "rg: found"
RG=rg
else
echo "rg: not found, will use grep"
RG=grep
fi
echo
echo "Searching for occurrences of process.chdir across repo..."
if [ "$RG" = "rg" ]; then
rg -n -C3 "process.chdir\(" || true
else
grep -RIn --line-number --binary-files=without-match -H "process.chdir(" . || true
fi
echo
echo "Showing lines 1108-1140 of test/bundler/expectBundled.ts (if present)"
if [ -f test/bundler/expectBundled.ts ]; then
sed -n '1108,1140p' test/bundler/expectBundled.ts || true
else
echo "File not found: test/bundler/expectBundled.ts"
fi
echo
echo "Searching for Bun.build occurrences..."
if [ "$RG" = "rg" ]; then
rg -n -C2 "Bun.build\(" || true
else
grep -RIn --line-number --binary-files=without-match -H "Bun.build(" . || true
fi
echo
echo "Searching for existing withCwd or __cwdQueue helpers..."
if [ "$RG" = "rg" ]; then
rg -n "withCwd|__cwdQueue" || true
else
grep -RIn --line-number -E "withCwd|__cwdQueue" . || true
fi
echo
echo "Listing process.chdir occurrences under test/ (if any)..."
if [ "$RG" = "rg" ]; then
rg -n --hidden --no-ignore --glob 'test/**' "process.chdir\(" || true
else
grep -RIn --line-number --include='test/*' "process.chdir(" . || true
fiLength of output: 108380
Serialize/avoid global process.chdir around Bun.build (prevents race)
- test/bundler/expectBundled.ts (lines 1125–1131): replace the manual process.chdir block with either the existing test harness cwdScope (test/harness.ts:1568–1576) for scoped cleanup, or — if tests run in parallel — wrap the operation in a global async mutex (e.g. withCwd(root, () => Bun.build(buildConfig))) to serialize chdir.
- Prefer removing chdir entirely by using absolute paths for Bun.build when possible.
2 participants
What does this PR do?
This branch:
Main:
This makes the bundler tests run about 60 seconds faster
How did you verify your code works?