test/bundler: make the three dead source map checks in expectBundled assert

[robobun]

Problem

Three checks in the external source map validation block of test/bundler/expectBundled.ts (the SourceMapConsumer.with callback, all three added together in #11344) cannot fail:

• snapshotSourceMap[...].mappings ends with expect(pos.line === dest.line); expect(pos.column === dest.column); (lines 1691-1692 on main). expect(boolean) with no matcher asserts nothing, so pos, the position the map actually produces for the entry, is never compared with anything. What an entry did verify is that its expected text exists at its expected line:col of the generated file, which only reads the generated file: a map that sends every token to the wrong place passes as long as the hard-coded position contains the quoted text, and a source position the map does not cover at all (generatedPositionFor returning null) passes too.
• The sourcesContent loop (lines 1669-1674) has the bound i < parsed.sources, a number compared with an array, so it never iterates. Had it iterated it would have read parsed.sourceContent (the field is sourcesContent) and resolved each source against the .map file path instead of the map's directory.
• The duplicate-mapping check (lines 1654-1662) runs for every external map, not only snapshotSourceMap users. Its fmtLoc reads every field except generatedLine from the mapping being visited (m) instead of from its argument, and generatedLine is part of the key the two mappings were matched on, so the two strings are always equal and the Duplicate mapping in source-map throw is unreachable.
• Users today: bundler_npm.test.ts (npm/ReactSSR) is the only mappings user; it and two tests in bundler_edgecase.test.ts set snapshotSourceMap; every external-map test with an outdir goes through the duplicate check. mappingsExactMatch is unaffected. The remaining dead check in this file, the never-populated testsRan duplicate-id set, is a different check and is fixed in test/bundler: make the itBundled duplicate-id check work and fix the 26 ids it finds #38471, which does not overlap with this diff.

Fix

• mappings: format what the map says for the entry's source position in the snapshot's own line:col:text shape (text sliced from the generated file at that position, "unmapped" when the map has no position for it) and toBe it against the entry. One string comparison covers position and text: equal strings mean the map's position is the expected one and the generated file has the expected text there, which is what the old text check established once the position agrees, so nothing that passed for the right reason changes. parseSourceMapStrGenerated now only validates the entry's format and returns its text. The assertion is labelled with the map file and entry, so a failure reads Expected: "8:0:console" / Received: "7:0:console" and the received value can be pasted back into the test; this also replaces the old text-mismatch path, which asserted and then threw a second Not matched error.
• sourcesContent: iterate sources.length, read sourcesContent, resolve against the map's directory, label the assertion with the map file and source.
• Duplicate check: fmtLoc formats its argument. Nothing else changes, so a map is rejected exactly when one generated position carries two different source positions, which is what the check's comment says it is for.
• expectBundled is exported again (it was exported until bundler tests, testing plugins #2740; expectBundled.md still documents calling it directly) so the test below can call it without registering anything.
• Every existing user holds under the live checks, so this is a harness fix only, no bundler change: bundler_npm.test.ts goes from 179563 to 179575 expect() calls (6 mappings entries, 6 sources) and passes; bundler_edgecase.test.ts (whole file), bundler_comments.test.ts, esbuild/loader.test.ts and esbuild/default.test.ts -t SourceMap pass with the duplicate check live, so no current output has a duplicate generated position. The odd-looking npm/ReactSSR entries are fine: at and or in "1:5623:at" / "23:4082:or++" are the minified names, and '<html>' -> "100:19062:void" is the last of the three generated ranges that position maps to, which is what generatedPositionFor returns for a multiply-mapped position, as before this change.
• Test: test/bundler/itBundled-snapshotSourceMap.test.ts. One itBundled case with correct mappings on a two-file bundle (this also runs the live sourcesContent loop on both sources), plus five cases that call expectBundled() directly and pin how the rejection starts: a position whose text is present but whose line the map disagrees with, a text mismatch, an unmapped position, a source rewritten after bundling via runtimeFiles so sourcesContent no longer matches disk, and a map rewritten in onAfterBundle to put two source positions on one generated position. Each case takes 0.2 to 0.6 s on a debug build. Against main's expectBundled.ts (plus the export), four of the five are accepted (Received: "<expectBundled() passed>") and the text mismatch fails only on the old unlabelled message; with this change all six pass.
• The fix lives under test/, so a fail-before check that only stashes src/ cannot observe it; the before/after above was run by swapping in main's expectBundled.ts under the debug build.
• Windows: the direct expectBundled() cases are describe.skipIf(isWindows) because its test/bundler/ stack check never matches backslash paths there, which is also why the itBundled case is silently not registered on Windows today (test/bundler: stop silently dropping every itBundled test on Windows #34552 fixes the check; the skip can go when it lands). Verified on Windows with the earlier spawn-based revision of this file that the harness registers nothing there, and that with test/bundler: stop silently dropping every itBundled test on Windows #34552's one-line fix applied the positive case passes un-skipped.

Background

• snapshotSourceMap is an itBundled option for source maps too large to snapshot whole. files pins the map's sources (and, through the loop fixed here, their sourcesContent); mappings is a list of [source position, generated position] samples, where the source side is "file:line:'token'" and the generated side is "line:col:text". Independently of that option, every external map produced by a test is decoded and checked for a generated position mapped to two different source positions.
• SourceMapConsumer.generatedPositionFor (the source-map package) answers "where did this source position end up in the output"; it returns { line: null, column: null } when the map has nothing at or before that position in that source.
• itBundled(id, opts) registers a test whose body is expectBundled(id, opts); expectBundled itself only bundles and runs the checks, returning a promise that rejects on the first failed check. runtimeFiles are written, and onAfterBundle runs, after bundling and before these checks, which is how the test makes a source file or the map disagree with the bundle.
• "AACA" is one VLQ segment: generated column +0, same source, source line +1, source column +0. Appended to the last mapped line it duplicates that line's last generated position with a different source line.
• expect(value, message) is bun:test's labelled form: the label replaces the matcher headline in the error message and Expected/Received are still appended (colored when attached to a terminal, hence the Bun.stripANSI in the test).

Error messages the test pins (debug build)

harness/SnapshotSourceMapWrongLine
  entry.js.map: generated position of entry.ts:2:'console'
  Expected: "8:0:console"
  Received: "7:0:console"
harness/SnapshotSourceMapWrongText
  Expected: "7:0:nope"
  Received: "7:0:cons"
harness/SnapshotSourceMapUnmapped
  entry.js.map: generated position of greet.ts:1:'greets'
  Expected: "1:3:greet"
  Received: "unmapped"
harness/SnapshotSourceMapStaleSourcesContent
  entry.js.map: sourcesContent of ../greet.ts
  (followed by the diff of the two contents)
harness/SnapshotSourceMapDuplicateMapping
  Duplicate mapping in source-map for 8:24
  8:24 -> 3:24 [/entry.ts]
  8:24 -> 4:24 [/entry.ts]

Earlier revisions of this PR

• First push: fixed only the mappings assertion; the test spawned a child bun test with the must-fail cases and grepped its reporter output, which took about 4 s on a debug build and needed a debug-only timeout.
• Second push: folded in the sourcesContent loop after review pointed out it is the same kind of dead check in the same block.
• Current: folded in the duplicate-mapping check for the same reason, and replaced the spawn with direct expectBundled() calls, which removed the child process, the env-var switch, the timeout and the dependence on reporter output.

[robobun]

^{Updated 7:05 AM PT - Aug 14th, 2026}

❌ @robobun, your commit 1e6cc67 has some failures in Build #95875 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38454

That installs a local version of the PR into your bun-38454 executable, so you can run:

bun-38454 --bun

[robobun]

Status: ready for review at fab0ef1 (rebased on current main); three LGTM bot reviews, no open threads.

CI: every lane that has run is green on both builds of this PR (95875 for the previous head, 96288 for this one: 177 of 179 jobs passed, 0 failed). The two remaining jobs in each build are the darwin 14 aarch64 - test-bun lanes, which have been waiting for an agent for over three hours in both builds, so the builds show as unfinished. The only annotations are flaky tests unrelated to this diff that passed on retry or when re-run alone (napi, inspect-error-leak, build-rust-toolchain-probe, sourcetextmodule-leak, postgres, test-cluster-shared-leak). The new itBundled-snapshotSourceMap.test.ts and the harness users (bundler_npm, bundler_edgecase, bundler_comments, esbuild/*) passed on every lane.

Reproduced by making the checks real and running the existing users: bundler_npm.test.ts goes from 179563 to 179575 expect() calls and still passes, and every external-map test passes with the duplicate-mapping check live, so the existing expectations and maps were right and the bug is the harness never checking them. The new test fails against main's expectBundled.ts (four of its five must-fail cases are accepted, the fifth fails on the old message) and passes with this change.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ed3a1580-e3ce-47b0-86ea-cc4ce1a0f0a7

📥 Commits

Reviewing files that changed from the base of the PR and between 5638c62 and fab0ef1.

📒 Files selected for processing (2)

• test/bundler/expectBundled.ts
• test/bundler/itBundled-snapshotSourceMap.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — test-harness-only fix that turns a no-op expect(boolean) into a real assertion, with a self-checking regression test. The inline nit about the adjacent sourcesContent loop is pre-existing and non-blocking.

Extended reasoning...

Overview

This PR touches only test infrastructure: test/bundler/expectBundled.ts (the snapshotSourceMap.mappings checker and its helper parseSourceMapStrGenerated) and a new test/bundler/itBundled-snapshotSourceMap.test.ts. No src/ changes. The old check ended in expect(pos.line === dest.line) with no matcher, so the source-map's actual generated position was never asserted; the fix formats generatedPositionFor's answer as line:col:text (or "unmapped") and .toBe()s it against the snapshot entry, with a labelled failure message.

Security risks

None. Changes are confined to the test harness and a new test file; no runtime, network, filesystem-outside-tempdir, or user-facing code paths are touched.

Level of scrutiny

Low-to-moderate. It's a small, mechanical harness correction whose blast radius is the three existing snapshotSourceMap users. The author verified bundler_npm.test.ts (the only mappings user) still passes with 6 more real assertions, and bundler_edgecase.test.ts -t SourceMap (the mappingsExactMatch users, unaffected) still passes. The simplified parseSourceMapStrGenerated keeps the same split.length != 3 guard, so no previously-accepted entry format is now rejected.

Other factors

• The new test is well-constructed per repo rules: spawns a child bun test on itself with an env flag to register must-fail cases, drains stdout/stderr/exited concurrently, spreads bunEnv and clears BUN_BUNDLER_TEST_FILTER, asserts each failure's exact label/Expected/Received plus 0 pass / 3 fail, and skipIf(isWindows) with a comment naming #34552. The debug-build timeout is commented.
• Checked that generated_lines[pos.line - 1] ?? "" and the bounded .slice() handle out-of-range positions without throwing, so the assertion always produces a diffable string rather than a TypeError.
• The one finding is a nit: a sibling dead loop (i < parsed.sources) four lines above, pre-existing since #11344. Fixing it also requires correcting sourceContent→sourcesContent and the path base, then revalidating ReactSSR — reasonable as a follow-up, not a blocker for this strict improvement.

[claude]

LGTM — thanks for also fixing the sourcesContent loop and adding the StaleSourcesContent case to prove it runs.

What was reviewed:

• The mappings assertion rewrite: generatedPositionFor result is now formatted as line:col:text and .toBe'd against the entry, with "unmapped" for null — covers the position, the text, and the previously-silent unmapped case in one comparison.
• The sourcesContent loop repair (bound, field name, path.dirname base) matches what I flagged; the runtimeFiles overwrite in StaleSourcesContent is a neat way to make it observably fail.
• Existing snapshotSourceMap users (npm/ReactSSR, two bundler_edgecase mappingsExactMatch cases) — the now-live loop reads the same on-disk files the bundler read, so verbatim match is expected; CI on 1e6cc67 covers it.
• parseSourceMapStrGenerated: the split.length != 3 guard was already there, so returning split[2] is equivalent to the old match.join(":").

Extended reasoning...

Overview

Test-harness-only change under test/bundler/. Two files: expectBundled.ts (repairs two vacuous assertions in the snapshotSourceMap block — the mappings position check and the sourcesContent loop) and a new itBundled-snapshotSourceMap.test.ts that self-verifies the harness by spawning a child that must fail four ways (wrong line, wrong text, unmapped position, stale sourcesContent). No production code (src/) is touched.

Prior feedback addressed

My earlier inline comment flagged the sibling for (let i = 0; i < parsed.sources; i++) loop as another never-runs assertion of the same class. Commit 1e6cc677 fixes all three defects I named (.length bound, sourcesContent field name, path.dirname base for resolving the source path) and adds a labelled expect plus a StaleSourcesContent rejected case that proves the loop now executes and can fail. That fully closes the feedback.

Security risks

None. Test infrastructure only; no runtime, network, auth, or crypto surface.

Level of scrutiny

Low-to-medium. The change strictly tightens a test harness — worst case it turns a previously-passing test red, which CI catches immediately and which would be a real finding about source-map fidelity rather than a defect in this PR. I checked the three pre-existing snapshotSourceMap users: npm/ReactSSR and the two bundler_edgecase mappingsExactMatch cases all set files, so the now-live sourcesContent comparison runs for them too; since it compares the map's embedded content against the exact on-disk file the bundler read, a mismatch would indicate an actual bundler bug.

Other factors

• parseSourceMapStrGenerated simplification is behaviour-preserving: the pre-existing split.length != 3 guard means ...match was always [split[2]], so match.join(":") ≡ split[2].
• The spawn test drains stdout/stderr/exited concurrently, spreads bunEnv, deletes BUN_BUNDLER_TEST_FILTER so an ambient filter can't hide the child's cases, asserts (fail) for each name plus 0 pass / 4 fail, and asserts exitCode last — all per harness conventions.
• The Windows skipIf is justified with a concrete cause (the test/bundler/ stack-path check vs backslash paths) and a tracked PR (#34552) that removes the need for it.
• The isDebug ? 60_000 : undefined per-test timeout is scoped to the one spawn test, not a file-wide default.

[claude]

LGTM — test-harness-only fix that turns three vacuous snapshotSourceMap checks into real assertions.

What was reviewed:

• Verified the new mappings path formats generatedPositionFor's answer as line:col:text and toBe's it against the entry, and that the now-live sourcesContent loop resolves against the map's directory and reads the correct field — the earlier note on this is addressed.
• Checked the fmtLoc closure fix (was reading m.* instead of loc.*) and that the DuplicateMapping case's ,AACA appendix produces two segments at one generated column to exercise it.
• Confirmed parseSourceMapStrGenerated's split.length != 3 guard was already present, so tightening from ...match to text drops no accepted inputs; only callers are in this file.
• The rejects suite now calls the newly-exported expectBundled in-process (superseding the spawn approach the PR body still describes) and asserts on the caught error message; ignoreFilter keeps an ambient BUN_BUNDLER_TEST_FILTER from turning them into no-ops.

Extended reasoning...

Overview

Two files under test/bundler/. In expectBundled.ts: (1) the snapshotSourceMap.mappings check replaces expect(pos.line === dest.line) no-ops with a single labelled expect(actual).toBe(generated_str) that compares the map's actual generated position and the text at that position; (2) the sourcesContent loop's bound is fixed from i < parsed.sources (array) to .length, the field name from sourceContent to sourcesContent, and the path base from the .map file to its directory; (3) the duplicate-mapping formatter's closure bug (m.* instead of loc.*) is fixed so the two sides actually differ; (4) expectBundled is exported and parseSourceMapStrGenerated is simplified to a validator that returns the text part. A new itBundled-snapshotSourceMap.test.ts has one passing bundle plus five in-process reject cases (wrong line, wrong text, unmapped, stale sourcesContent via runtimeFiles, duplicate mapping via onAfterBundle) that assert on the caught error message.

Security risks

None. Test-harness code only; no production/runtime code touched, no external input handled.

Level of scrutiny

Low-to-medium. This is test infrastructure with three existing consumers (bundler_npm.test.ts ReactSSR and two bundler_edgecase.test.ts cases), all of which the PR body reports still pass with more expect() calls. The main risk — that making the assertions real breaks an existing user — was checked by the author and is easy to verify in CI. I traced that the two bundler_edgecase users only set mappingsExactMatch (unchanged path) plus files, so the newly-live loop adds three sourcesContent reads there; the npm/ReactSSR user exercises both fixed paths.

Other factors

• My prior inline comment about the sibling sourcesContent loop was fully addressed and is marked resolved.
• The split.length != 3 guard in parseSourceMapStrGenerated already existed, so switching from ...match/join(':') to a plain text destructure cannot reject anything previously accepted.
• The reject cases hard-code exact generated positions (e.g. 8:24 -> 3:24); the file-top comment documents the expected 8-line output so a future bundler-output change produces a readable diff rather than a mystery failure.
• The PR description still describes a spawn-based child-process test; commit fab0ef1 moved to in-process expectBundled calls (hence the new export). Description staleness only, code is coherent.
• Windows: the reject block is skipIf(isWindows) because expectBundled's test/bundler/ stack check never matches backslash paths (tracked in #34552); the passing itBundled case is silently dropped there for the same pre-existing reason, which the test comments.