Conversation
…d pin output shapes Every case builds with the CLI backend and the describe block is describe.concurrent, so the build and run subprocesses of up to five cases overlap instead of running one at a time. The file takes 24s instead of 77s under the debug ASAN build. Fifteen cases whose comments describe an output shape now pin that shape with an inline snapshot of the bundle body (runtime helpers and chunk hashes stripped) instead of substring checks. The runs of the six cases that exercise a caught throw also expect an empty stderr. The two ThenableImportee cases become one loop over the splitting flag.
|
Status: test-only change, no Reproduced: With this branch: 24.1s, 24.2s, 24.0s, 164 pass, 15 inline snapshots. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe pull request updates dynamic-import DCE tests to use CLI-backed concurrent execution, normalized inline snapshots, exact export-shape checks, and explicit runtime error assertions. ChangesDynamic import DCE
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change only updates one test file to run cases concurrently and pin export shapes; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, verification results, performance impact, and test coverage. It does not use the exact template headings, but it provides the required 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 refactor that strengthens assertions and reuses an established concurrency pattern.
What was reviewed:
- The
backend: "cli"wrapper matchesbundler_compile.test.ts/esbuild/importstar.test.tsverbatim, andexpectBundled.ts:1972confirms only API-backend cases are forced toit.serial, sodescribe.concurrentis safe here. bundleBody's^// [\w./-]+$correctly skips// @ bunand the hash regex only touches-[hash].jschunk names;jsOutputssorts and CRLF-normalizes for stable cross-platform snapshots.- Collapsed
ThenableImporteeloop: addingoutdir: "/out"+run.fileto the no-split case is consistent with other non-splitting cases in this file that already useoutdir. - No case is deleted or weakened — every prior
.not.toContain("DROPPED")/runcheck is subsumed by the inline snapshot or kept as-is.
Extended reasoning...
Overview
This PR refactors a single bundler test file, test/bundler/bundler_dynamic_import_dce.test.ts, with no production code changes. It (1) wraps itBundled to default backend: "cli" so cases can overlap under describe.concurrent, (2) replaces ~15 loose .toContain / .not.toContain substring checks with toMatchInlineSnapshot over a normalized bundle body (new jsOutputs / bundleBody / outputBodies helpers that sort outputs, normalize CRLF, strip the runtime-helper prelude, and mask 8-char chunk hashes), (3) adds stderr: "" to six run: specs that exercise caught throws, (4) collapses two identical ThenableImportee cases into a for (const splitting of [true, false]) loop, and (5) adds an onAfterBundle snapshot to one previously assertion-less case.
Security risks
None. This is test-only code that spawns the debug bun build binary against per-case temp directories via the existing expectBundled harness. No network, no auth, no crypto, no user-facing surface.
Level of scrutiny
Low-to-moderate. The concurrency change is the only part with correctness implications, and it reuses the exact const itBundled = (id, opts) => itBundledBase(id, { backend: "cli", ...opts }) wrapper already committed in bundler_compile.test.ts and esbuild/importstar.test.ts. I verified in expectBundled.ts that resolveBackend returns opts.backend when set and that only "api" cases are registered with it.serial (because of the process.chdir around Bun.build), so CLI-backend cases legitimately overlap under describe.concurrent. Each case writes to its own harness temp root, so there's no shared filesystem state.
Other factors
REVIEW.md's "never silently weaken an existing test" and "assert the strongest invariant" both cut in favor of this change: every replaced substring check is subsumed by a full inline snapshot of the narrowed export object / chunk, and every run: block is retained. The bundleBody regex /^\/\/ [\w./-]+$/m correctly excludes // @ bun (target-bun prelude) since @ isn't in the character class, and the -[a-z0-9]{8}\.js hash mask matches the same pattern bundler_splitting.test.ts strips. The collapsed ThenableImportee loop adds outdir: "/out" to the no-split variant, which is consistent with many other non-splitting cases in this file already using outdir + run.file. No CODEOWNERS entry covers test/bundler/, and there are no outstanding reviewer objections in the timeline. Exit reason was dry_streak.
This branch has not been deployed
1 participant
Problem
test/bundler/bundler_dynamic_import_dce.test.tstakes 27s on the x64-asan lane (build 108977): 164 cases in sequence, 151 with a bun subprocess.describe.concurrentalone does nothing: API-backend cases register asit.serialbecause the harness chdirs aroundBun.build().DROPPEDsentinel is absent, so a regression in the shape of the export object passes.Fix
describe.concurrent. ASANbun testoverlaps 5 cases.ThenableImporteecases become one loop. Everyrunstays, nothing is deleted or skipped.bun bd test test/bundler/bundler_dynamic_import_dce.test.ts(debug ASAN): 79.2s, 75.7s, 76.3s before, 24.1s, 24.2s, 24.0s after. CI build 109038: 8.6s on x64-asan, every lane green.expectBundled.tsis untouched.expectBundled.ts) to per-file wrappers: a follow-up that deletes this wrapper. AlsoitBundled.only/.skipare unavailable (unused here) andBun.build()no longer runs these cases (the linker is shared).Background
itBundled(test/bundler/expectBundled.ts) builds in-process withBun.build()by default, or with abun buildsubprocess underbackend: "cli". Only CLI cases overlap.run: { stdout }spawns bun on the output and requires exit code 0 and an exact stdout. It checks stderr only when set.splitting, the importee is a[name]-[hash].jschunk whoseexport { }list is the narrowed set. Without it, itsexports_xobject is narrowed.Notes
Every
runstays because it is the only positive check: the text checks prove a dropped export is absent, the run proves the kept exports still resolve. Removing a run would let over-shaking pass.CI build 109038, this file per lane: x64-asan 8.59s (27.4s in build 108977, 19.8s in
test/expected-durations.json), debian x64 1.62s, alpine x64 1.10s, Windows 2019 x64 2.11s, Windows 11 aarch64 3.46s, darwin aarch64 2.31s. 164 pass and 15 snapshots on each.Timings in this container (16 vCPUs), same machine for before and after, all green. Per case, debug ASAN: 350ms to 1100ms serial before (the run subprocess is most of it). After, a case takes 500ms to 1000ms wall because a
bun buildsubprocess is added and 5 cases share the CPU, but 5 run at once.The released bun (1.4.1) does not have the
import()tree-shaking these cases cover (93 of 164 fail on main withUSE_SYSTEM_BUN=1), so there is no meaningful release-build timing.The CLI path fails a case on an unexpected bundler warning. No case here warns. Options this file uses and how the CLI branch handles them:
splitting,format,target,outdir,minifySyntax,define(already CLI on main because ofresolveBackend),entryPoints,runtimeFiles,bundleErrors(parsed from stderr,UnwrapCjsAssignStillErrorspasses). Every case already wrote to its own directory under the harness temp root, and everynode_modulesfixture is a file the case writes. Nothing contacts the network.bundleBody(): the first line matching^// [\w./-]+$is the first module comment (// b.js,// node_modules/lib/index.js).// @bun(target bun) does not match, so it is dropped with the helpers. The chunk hash is 8 chars, the same patternbundler_splitting.test.tsstrips.jsOutputs()sorts the file names so a multi-file snapshot has a stable order. A snapshot holds only the narrowed__exportobject, the chunk'sexport { }list, and the import sites.Snapshot cases: AwaitDestructure, TwoSitesUnion, BailoutRest, SplittingNarrowedExports, SplittingTwoImportersUnion, WebpackExportStarAsBehindImport, SplitRequireDestructureNarrows, NoSplitRequireNarrows, RolldownAwaitDestructPartial, RolldownUnusedDynamicImportedChunk, RolldownInlineDynamicImportsThenNarrow, InlineThenRejectHandlerBails, InlineLetDestructureUnused, InlineDuplicateDestructureKeyBails, InlineThenWrappedSiblingExportsRefLive (this one had a run only). Each snapshot replaces the substring checks it subsumes. The other substring checks stay: a full snapshot of every case would make any printer change touch 150 snapshots.
stderr: ""cases: MinifyInlinedNamespaceLocalEscapes (5 shapes), WebpackCallingNamespaceKeepsAll, InlineThenRejectHandlerBails, NoSplitTryCatchCatches, NoSplitTryAwaitThenCatches, InlineThenCatchChainBails, InlineThenFinallyCatchChainBails.Other near-identical pairs (
Splitting/InlineExportedDestructureKept, ThenFunctionArgumentsBailout, ThenRejectHandler, ThenCatchChain) differ in their inputs or expectations, so they stay separate cases.Also ran prettier on the file.
[auto-merge] 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
self-review · 6 surviving concerns
process.chdir+configRefwindow aroundawait Bun.build()at expectBundled.ts:1306-1338 that forcesit.serialat :1972), and a ~15-line promise-chain mutex in expectBundled.…backend: "cli"wrapper is a workaround for a 12-line harness gap (it.serialon API cases at expectBundled.ts:1972) that a measured harness-level lock beats by ~25% with one fewer ASAN child per case…it.serialline in expectBundled.ts; a 4-hunk promise-chain mutex in the harness is faster on this very file under the ASAN cap (20.5s vs 26.0s), keepsBun.build()coverage, fixes everydescribe.concurrentbundl…backend: "cli"wrapper that reverses Make the bundler tests use the API by default in most cases #22646's API-by-default decision file-by-file and adds 15 inline snapshots that will c…31 concerns were raised and did not survive verification.