Conversation
|
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 (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughChangesThe bundler default tests now use a CLI-default wrapper with API overrides. They add generated-output and runtime assertions for module resolution, exports, dynamic imports, scope handling, entry naming, top-level await, CommonJS globals, and minification. Bundler test validation
Merge Risk: ⚪ Minimal · up to This PR reorganizes and strengthens bundler tests while reducing their runtime; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, verification commands, test results, performance impact, and scope. It does not use the exact template headings, but it fully covers both required topics. Comment |
There was a problem hiding this comment.
LGTM — test-only hardening that turns assertion-light bundler cases into ones that can actually fail, plus a CLI-backend default for concurrency.
Checked the itBundled wrapper: backend is a real BundlerTestInput field, the spread order lets backend: "api" override the default, and the callback form is preserved. The assert(fs, fs2) → strictEqual/deepStrictEqual rewrites now compare real values instead of a truthiness check with a message arg. The new onAfterBundle output-shape checks (scanImports externals, require.main/__require., var-relocation, new (require_foo()).Foo parenthesization) read as correct against the fixtures they cover. No runtime code, no CODEOWNERS on this path.
Extended reasoning...
Overview
This PR touches a single file, test/bundler/esbuild/default.test.ts, which is the esbuild-ported bundler test suite. It makes two kinds of changes: (1) a local itBundled wrapper that defaults every case to backend: "cli" so the describe.concurrent block actually runs concurrently (the API backend uses it.serial because it calls process.chdir), with one case (HashbangBannerUseStrictOrder) explicitly opting back into backend: "api" due to a CLI --banner quoting quirk; and (2) a large number of assertion additions — converting run: true to run: { stdout: "" }, replacing the vacuous assert(fs, fs2) with real strictEqual/deepStrictEqual comparisons, and adding onAfterBundle output-shape checks (parenthesization of new (require_foo()).Foo, export-chain collapse, dynamic-import preservation, scanImports on externals, require.main/require.cache survival, var-relocation hoisting, etc.). No runtime or bundler source is touched.
Security risks
None. This is a test-only change to a bundler regression suite. It adds no network access, no credentials, no file-system writes outside the harness's temp dirs, and does not touch any security-sensitive path (auth, crypto, TLS, package extraction). The Bun.Transpiler().scanImports calls operate on the bundler's own output in-process.
Level of scrutiny
Low-to-moderate. Test-only changes that strengthen assertions carry the risk of pinning to incorrect output (REVIEW.md: "a snapshot captured from buggy code certifies the bug"), so I read each new onAfterBundle check against its fixture. The new (require_foo()).Foo parenthesization, c as a alias collapse, await import(name) preservation, export * from "fs" / require("fs") presence, arguments survival under minification, and __require. absence all match what the bundler should emit for those inputs. The itBundled wrapper was checked against expectBundled.ts — backend?: "cli" | "api" is a real option, the spread order ({ backend: "cli", ...opts }) correctly lets per-case overrides win, and the callback-form branch preserves the metadata argument. The .skip static is forwarded via Object.assign.
Other factors
The bug hunt ran to dry_streak with no findings. No CODEOWNERS entry covers test/bundler/. There are no prior review comments or outstanding objections in the timeline. The PR description reports the full suite passes on debug+ASAN, release, and Windows canary with the same 152 live cases and no new todos/skips, and the change directly implements REVIEW.md's "every assertion must be able to fail" and "prefer concurrent tests" guidance. The one intentional behavior difference — the strengthened HashbangBannerUseStrictOrder assertion now checking both the file hashbang and the banner line — is a strict tightening of the previous startsWith("#! in file") check.
… cases --no-bundle, drop the stale leaksan entry
Problem
test/bundler/esbuild/default.test.tstakes 13s to 14s on the debian 13 x64-asan lane (build 108747), as a serial step:test/no-validate-leaksan.txtlists it. Inside the file, 128 of 152 live cases build throughBun.build()in-process, whichexpectBundledregisters withit.serial, and every serial case is a barrier for the 24 concurrent ones.*NoBundlecases bundled.Fix
itBundledto the cli backend, asbundler_compile.test.tsdoes. 127 cases move with their options and assertions unchanged.HashbangBannerUseStrictOrderstays on the api backend (the cli harness quotes--bannervalues) and usesiife, so the hashbang, banner and"use strict"order is observable.detect_leaks=1env. The three*NoBundlecases andVarRelocatingNoBundlegetbundling: falselike upstream. The last one becomes todo: the harness has no multi-entry--no-bundle, and it was a copy of the bundle case.ExportWildcardFSNodecomparisons, four new runs,onAfterBundlechecks on 18 cases.bun bd test test/bundler/esbuild/default.test.ts(debug, ASAN) 53.9s to 54.9s before, 20.4s to 21.0s after, 151 pass. Released bun 1.25s to 0.31s, Windows canary 2.32s to 0.73s. Self-reviewed: 8 concerns, 3 addressed, the harness-level alternative is in Notes.Background
itBundledpicks the backend from the options. Withoutdotenv,production,bundling: false,define,env,bundleWarnings,emitDCEAnnotationsorrun.validateit callsBun.build()in the test process, else it spawnsbun build. Only the in-process backend is serial: it wraps the build inprocess.chdir.Notes
Timings on one 16-core container. Debug is
bun bd test(debug build with ASAN). Released isUSE_SYSTEM_BUN=1 bun testwith bun 1.4.1. Windows is the canary bun 1.4.1 on a Windows Server 2019 machine,USE_SYSTEM_BUN=1, first revision of this branch.The per-test durations on main add up to 54.2s for a 54.5s run. After the change they add up to 77.8s for a 20.6s run: each cli case spawns
bun buildas well as the run child, and 5 cases overlap. The remaining cases above 1s have four run steps each (ConditionalImport,ConditionalRequire) or importfsin node mode.Leaksan: the entry dates from #21142 (September 2025, group
Zig::SourceProvider::~SourceProvider()). WithBUN_DESTRUCT_VM_ON_EXIT=1,ASAN_OPTIONS=...detect_leaks=1:abort_on_error=1and thetest/leaksan.suppsuppressions, the debug ASAN build runs the whole file (children included,bunEnvspreadsprocess.env) with exit 0 and no report, twice.scripts/runner.node.mjs(isBucketCandidate) requiresshouldValidateLeakSanon the asan lane, so the file now joins the parallel bucket there. The other 14 files of that group are untouched. If the asan lane reports a leak this revision did not see locally, the line comes back.Self-review: eight concerns. Addressed:
HashbangBannerUseStrictOrdernever reached the directive branch underesm(bun drops"use strict"there; upstream usesFormatIIFE), the three*NoBundlecases that bundled, theVarRelocatingNoBundlecopy, the leaksan entry. Not done here, on scope: the serial barrier lives inexpectBundled.ts(process.chdirand the module-globalconfigRefaroundawait Bun.build, thenit.serial). A promise-chain mutex around that window plus plainitwould let every api case of everydescribe.concurrentbundler file overlap while it keepsBun.build()coverage, and a prototype of it ran the unmodified file in 17.0s against 20.2s for this flip. The handoff for this task keepsexpectBundled.tsout of this PR (#38454, #40497 and #38471 edit it), so that is a follow-up. With it, the wrapper here and the one inbundler_compile.test.tsbecome unnecessary. Also a follow-up:run: truecould default tostdout: ""in the harness.Backend counts: the brief counted the 44 option-forced cases as the serial api ones.
resolveBackendreturns"cli"for those options, so they were the concurrent ones. 24 of them are live, the other 20 aretodoor use an option the harness does not implement.Assertion changes:
stdout: ""on the 17 run steps that had no expectation. Their fixtures assert withnode:assertand print nothing:NewExpressionCommonJS,ExportFormsES6,ExportFormsWithMinifyIdentifiersAndNoBundle(todo: multi-entry--no-bundle),ImportFormsWithNoBundle,ImportFormsWithMinifyIdentifiersAndNoBundle,ExportFormsCommonJS,ExportChain,AwaitImportInsideTry,NestedScopeBug,ExportFSBrowser,ExportFSNode,ReExportFSNode,ExportFSNodeInCommonJSModule,ExportWildcardFSNodeES6,ExportWildcardFSNodeCommonJS,ThisOutsideFunctionNotRenamed,ThisInsideFunction.ExportWildcardFSNodeES6andCommonJS:assert(fs, fs2)becomesreadFileSyncidentity plus equal key sets (minusdefault). The es6 output must holdexport * from "fs", the cjs outputrequire("fs").ArgumentsSpecialCaseNoBundleprintsmarkerand executes its 50assert.deepEqualcalls (asout.cjs:var argumentsneeds sloppy mode).NamedFunctionExpressionArgumentCollisionprints123.ArrowFnScoperuns atest.jsthat calls the four arrows without defaults (tests[0](1, 2) === 3, ...) and checks that the comma expressions assigned the globalx.NonDeterminismESBuildIssue2537importsaapand checksaap(false, 4) === "teun"andaap(true, 4) === 10.onAfterBundlechecks:NewExpressionCommonJSkeepsnew (require_foo()).Foo.ExportChainexportsc as aand has nobbinding.AwaitImportInsideTrykeepsawait import(name).NestedScopeBugkeepsb()andvar bunder one name.HashbangBannerUseStrictOrderstarts with#! in file,#! from banner,"use strict";(before: only the file hashbang).ArrowFnScope: the globalsx,y,zkeep their names and no arrow has a parameter named like them.ArgumentsSpecialCaseNoBundle: 8(x = arguments)defaults and 3var argumentssurvive minification.ExternalModuleExclusionPackage,ScopedExternalModuleExclusion,ExternalWildcardDoesNotMatchEntryPoint: exact import list viaBun.Transpiler.scanImports.ManyEntryPoints: all 40 outputs hold their ownshared_default = 123and noimport.TopLevelAwaitNoBundlekeepsawait fooandfor await.RequireMainCacheCommonJSkeepsrequire.mainandrequire.cacheand has no__require.member.VarRelocatingBundle:for (var i = 1 in {})becomesi = 1;plus the loop, a block-levelfunction lis lowered tovar l, the one in a function body stays.EntryNamesNoSlashAfterDir: each output holds itsconsole.log(n).NonDeterminismESBuildIssue2537: the export keeps the nameaap, the local is minified.Found on the way, not asserted here because the case would fail:
require_entryand never calls it (bundler: call the wrapped entry point in iife output #37843), and__requireis not defined in iife output (bundler: define __require in iife output #38077). This is whyArgumentsSpecialCaseNoBundleruns as--no-bundleonly.ImportNamespaceThisValue: in cjs output the call of a named import from an external is printed asimport_external.foo(), sofooruns withthis === import_external. esbuild prints(0, import_external.foo)(). A probe with a runtimeexternalpackage confirms the leak, also under node.ToESMWrapperOmission:bun build --no-bundle --format=cjsleaves the ESM syntax in place, so the case cannot check the wrappers it is named for.WarnCommonJSExportsInESMBundle: thecjs-in-esm.jsoutput references an undeclaredmodule_cjs_in_esm. js_parser: make the ESM/CJS classification and the module/exports bindings agree #40840 works in that area.StrictModeNestedFnDeclKeepNamesVariableInliningESBuildIssue1552: its output changes with bundler: implement --keep-names with __name helper #40503 (--keep-names), so nothing is pinned.MetafileNoBundle,MetafileVariousCases,MetafileVeryLongExternalPathsaretodothrough harness limits (multi-entry--no-bundle,copyanddataurlloaders).todo: truecases are test: enable stale todo tests that now pass #39058's job. No case was removed or skipped.timeoutScaleis not used in this file. No case spawns a child of its own, except two todo cases that run esbuild on their output.BUN_BUNDLER_TEST_USE_ESBUILD=1(first revision): 79 failures on main, 19 here. The esbuild api backend of the harness forwards almost no options, the cli backend does. Seven cases pass on main and fail here with esbuild: five pin bun's printer output or run through the api backend (NestedScopeBug,VarRelocatingBundle,VarRelocatingNoBundle,RequireMainCacheCommonJS,HashbangBannerUseStrictOrder), andInjectDuplicateandRequireResolvenow reach esbuild with their real options.The same four
tscerrors exist on main and here (automaticRuntime,copyanddataurlloaders). prettier reports the file clean.Related open PRs that edit this file (#40840, #40857, #40804, #39058, #38471 and others) add or flip cases in other hunks.
expectBundled.tsis not touched (#38454, #40497). #38385 editstest/no-validate-leaksan.txtin other lines.First revision (ed744b6): the banner case pinned only the first two lines under
esm, the three*NoBundlecases bundled,VarRelocatingNoBundleran the bundle checks, and the leaksan entry stayed. 152 pass, 405 expect() calls.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file