Repository navigation
test(bundler): run esbuild/importstar.test.ts concurrently and tighten its assertions - #41028
Conversation
…n its assertions Default every case to the CLI backend and switch the describe block to describe.concurrent. itBundled registers API-backend cases with it.serial, so the file ran its 75 cases one at a time. Add runtime and output checks to the cases that only checked that the build succeeds, and give ImportStarMangleNoBundleNoCapture the no-capture entry point it was ported for.
|
Status: ready for review. This PR only changes Reproduced the slow run locally with |
|
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; 6 remain after this review. WalkthroughThe import-star bundler tests now use the CLI backend and concurrent execution. They add assertions for omitted namespace helpers, ES module/CommonJS behavior, direct namespace property access, IIFE self-re-exports, and CommonJS namespace serialization. ChangesImport-star bundler test coverage
Merge Risk: ⚪ Minimal · up to This test-only change runs the import-star cases concurrently and strengthens their assertions, with the full test file passing repeatedly and completing faster. 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 clearly explains the problem, implementation, verification steps, performance results, scope, and known limitations. It does not use the exact template headings, but it includes the required purpose and verification information. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only change following the established bundler_compile.test.ts wrapper pattern, with strictly tighter assertions.
What was reviewed:
- Concurrency safety:
expectBundled.tskeys each case's root dir on its uniqueidand only the API backend touchesprocess.chdir/configRef, so CLI cases overlap cleanly underdescribe.concurrent(matches 15 other bundler test files already doing this). ImportStarMangleNoBundleNoCaptureinput change: the old body was a copy-paste of the Capture case; the newns.foo, ns.foobody matches the other*NoCapturetests in this file and the Capture variant still exists unchanged, so no coverage is lost.- New
onAfterBundle.not.toContain("__export"/"__commonJS"/"__reExport")checks and the addedrun:/runtimeFilesblocks mirror existing patterns in this file andesbuild/dce.test.ts/splitting.test.ts.
Extended reasoning...
Overview
This PR touches a single file, test/bundler/esbuild/importstar.test.ts. It (1) wraps itBundled to default every case to backend: "cli" and switches the outer describe to describe.concurrent, (2) fixes ImportStarMangleNoBundleNoCapture, which was previously byte-identical to the Capture variant, to actually use a non-capturing entry point, and (3) adds onAfterBundle assertions that runtime helpers (__export, __commonJS, __reExport) are absent plus run:/runtimeFiles stdout checks to four cases that previously only asserted the build succeeded. No production code changes.
Security risks
None. This is a test-only change to a bundler test file; it introduces no new inputs, no network access, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The wrapper is byte-identical to test/bundler/bundler_compile.test.ts:13, and describe.concurrent is already used across 15+ bundler test files. I verified in expectBundled.ts that (a) each case writes to path.join(tempDirectory, id) so concurrent cases never share a directory, and (b) only the API backend uses process.chdir and the module-global configRef — the harness itself registers API cases as it.serial for exactly this reason (line 1962-1965). Forcing CLI here does not drop meaningful Bun.build coverage: these are esbuild-ported semantic tests of bundler output, not JS-API-surface tests, and the bundler codepath is shared.
Other factors
The one input mutation (ImportStarMangleNoBundleNoCapture) is a fix to a mislabeled duplicate rather than repurposing a test — the Capture variant remains at line 325 with its original JSON.stringify(ns) assertion, so REVIEW.md's "never mutate an existing test's input to the new case" is satisfied. The new .not.toContain helper-name checks are stable bundler runtime symbols (used the same way elsewhere in the suite) and each is commented with the invariant it locks in. The new runtimeFiles/run: { file: "/test.js" } addition to ExportOtherAsNamespaceCommonJS copies the exact pattern from the adjacent ImportExportOtherAsNamespaceCommonJS case. No CODEOWNERS entry covers this path, the timeline shows no prior reviews or objections, and the bug-hunt exit reason was dry_streak.
Problem
test/bundler/esbuild/importstar.test.tstakes 11s on the debian 13 x64-asan lane (build 108747). The defaultitBundledbackend isapi, which the harness registers withit.serial(expectBundled.ts:1962), so its 75 cases run one at a time. 64 of them spawn a subprocess forrun.ImportStarMangleNoBundleNoCaptureis a copy of the Capture case.Fix
bundler_compile.test.ts) and usedescribe.concurrent. Error and warning cases pass on it unchanged.ImportStarAndCommonJSruns the bundle, no__commonJSwrapper.ExportSelfIIFEruns the bundle, no__reExporthelper.ImportExportOtherAsNamespaceCommonJSrequires the bundle and checks the JSON.ImportStarNoCaptureandImportStarExportStarNoCapture: no namespace object (__export).ImportStarMangleNoBundleNoCapturegets the upstream no-capture entry point.bun bd test test/bundler/esbuild/importstar.test.ts(debug, ASAN): 35.3s and 35.4s before, 12.4s to 14.5s after (six runs). Released bun: 0.7s before, 0.25s after. 75 pass, 4 todo, every run. expect() calls: 64 before, 71 after.Background
apibackend callsBun.build()in the test process withprocess.chdir, so the harness makes it serial. Theclibackend spawnsbun buildin the case's own directory and can overlap.bun testruns consecutive concurrent cases as one group of up to 20. A serial case ends the group, so the no-runcases also usecli(onapithey measured 15.4s).__exportbuilds a namespace object.__commonJSwraps a CommonJS module.__reExportcopies exports at runtime forexport *of an external module.Notes
What was not changed, and why:
ImportStarExportStarAsNoCaptureandImportStarExportImportStarNoCaptureget no__exportcheck. Bun still materializes the namespace forns.foothrough a re-export (esbuild bindsfoodirectly). bundler: bind property accesses on re-exported namespaces directly #41009 changes that and adds cases at the end of this file. This branch does not touch those lines.ImportDefaultNamespaceComboNoDefault1to4stay separate builds. Bun reports only the first entry point's error, so one build with four entry points would check one error instead of four.timeoutScale.entry.js,foo.js,bar.js).Other checks:
bun bd test ... --todo: the 4 todo cases still fail, as on main.BUN_BUNDLER_TEST_USE_ESBUILD=1: main fails 32 cases because the API backend passes neitherformatnorexternaltoesbuild.build(). This branch fails 2:ImportStarCommonJSCapture(esbuild addsdefaultto the CommonJS namespace, noted in the case) andReExportStarEntryPointAndInnerFileExternal(bundler: fix re-exports of the "bun" builtin with --target=bun #37829).nsinImportStarNoCapturemakes__exportappear and the case fails. A CommonJSfoo.jsinImportStarAndCommonJSmakes__commonJSappear and the case fails.itBundledimport) also changes in bundler: emit namespace binding for runtimeexport * from <external>#36714 and js_printer: parenthesize missing-import void 0 on LHS of ** #36134 (they addexpectto line 1). The merge conflict is those two lines.Ran 79 testsline, host load average about 30). Released bun: 0.78s and 0.67s before, 0.25s and 0.24s after.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file