Skip to content

bun:test: show a carriage return as ␍ in the diff of a failing matcher - #47816

Open
robobun wants to merge 4 commits into
mainfrom
robobun/3c1e59ee/show-cr-in-matcher-diff
Open

bun:test: show a carriage return as ␍ in the diff of a failing matcher#47816
robobun wants to merge 4 commits into
mainfrom
robobun/3c1e59ee/show-cr-in-matcher-diff

Conversation

@robobun

@robobun robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Problem

  • A failing matcher prints an empty diff when two strings differ only in line endings: expect("a\r\nb").toBe("a\nb") prints - Expected - 0 / + Received + 0. Jest and Vitest print a changed row.
  • The test printer's string arm (src/runtime/test_runner/pretty_format.rs:1206) and DOM printer write \r\n and \r as \n, also in a failure diff.
  • Regression from Update bun:test diff #21158 (1.2.20).

Fix

  • Formatter.normalize_newlines is off by default. Only the two producers of stored Bun-format snapshots set it.
  • The diff printer draws every \r as ␍ (U+240D).
  • Stored snapshot text does not change: 11,000 generated snapshots are byte-identical. Messages without a diff do not change.
  • Verified: test/js/bun/test/printing/diffexample.test.ts and three more files (5 tests fail on main), snapshot-tests/, expect.test.js. Self-reviewed: 19 concerns raised, 17 addressed.

Background

  • DiffFormatter::new prints both values. print_diff_main diffs them by line.
  • The same printer writes Bun-format snapshots, which store \n for every line ending (jest-snapshot's normalizeNewlines).
  • Considered the raw \r, as in Jest: a terminal overdraws the row, and a GitHub annotation drops the byte. Considered the two characters \r: a backslash prints as it is, so "a\\r" prints the same.

Downsides

  • A diff with a \r changes: ␍ shows where main broke the line or printed nothing. Stored copies need an update.
  • A literal ␍ in a value draws like a \r: only row marks and colours tell them apart. expect.toSatisfy() still rewrites \r in its message.
  • A passing Bun-format snapshot pays +6.3 instructions per string (+0.26%). A failing diff without \r pays +118 to +331 (+0.4% to +1.0%). Release .text grows 1,024 bytes.
Notes

Messages (no colours). No issue reports this. The empty diff shows in every release from 1.2.20 to 1.4.4.

assertion main this PR Jest 30.5.2 / Vitest 5.0.5 (<CR> is the raw byte)
expect("a\r\nb").toBe("a\nb") "a / b", footer 0/0 - "a / + "a␍ / b", footer 1/1 - a / + a<CR> / b
expect({ s: "a\r\nb" }).toEqual({ s: "a\nb" }) value once, footer 0/0 - "a / + "a␍ in the object rows, 1/1 - "s": "a / + "s": "a<CR>
expect("a\rb").toBe("a\nb") footer 0/0 - "a / - b" / + "a␍b", 2/1 - a / - b / + a<CR>b
expect("ab\r").toBe("ab") - "ab" / + "ab / + " Expected: "ab" / Received: "ab␍" the same two lines, <CR> raw
expect("x\r\na\r\ny").toBe("x\r\nb\r\ny") "x / - b / + a / y" "x␍ / - b␍ / + a␍ / y" x<CR> / - b<CR> / + a<CR> / y
this.utils.diff({ s: "a\nb" }, { s: "a\r\nb" }) Compared values have no visual difference. a 1/1 diff a 1/1 diff
expect([new Error("a\r\nb")]).toEqual([new Error("a\nb")]) + [Error: a<CR> + [Error: a␍ <CR> raw
  • The same change reaches toStrictEqual, toMatchObject, toHaveProperty, the CalledWith, ReturnedWith and ResolvedWith matchers, .resolves / .rejects, the Expected: not line, UTF-16 strings, and DOM text, comments and attribute values. It reaches a string inside an array, an object, a Map, a Set, and the sample of stringContaining, stringMatching, objectContaining and a custom matcher.
  • ␍ is drawn for every producer of a diff, also for the ones that wrote the raw byte before: an Error message, a Symbol description, the characters of a String object, a 16-bit property key, this.utils.diff of two strings, and the diff of a snapshot that does not match.
  • Jest and Vitest write the byte raw. A first version of this PR did too. A terminal draws Received: "ab<CR>" as "eceived: "ab, and github_action_writer drops the byte, so the annotation read Expected: "ab" / Received: "ab".
  • Bun 1.2.19 printed Received: "a\r\nb". Its diff used the console formatter, which escapes a backslash too.
  • The diff compares the bytes as they are. Only the text of a row is drawn with ␍. So expect("a\r\nb").toBe("a␍\nb") prints - "a␍ / + "a␍ / b" with a 1/1 footer, and not an empty diff.
  • With colours, a \r that is the only change has the inverse style of a whitespace-only change. A ␍ of the value has the colour of its row.
  • A message without a diff (toContain, toMatch, toBeNull and the like) does not change. make_formatter prints it with the console.log formatter, which writes a \r as the two characters \r. Of 35 failing assertions on values with a \r, 33 have no diff, and those messages are byte-identical on main and here.

What is not in this PR

Open PRs on the same lines

Self-review: not taken

Measurements (main 6a38049 against this PR, linux x64. perf, valgrind, strace and bloaty are not installed here. The snapshot comparison ran on the base before, 271b65e, with the same printer code.)

  • Tag::String arm, instructions per snapshot string (release, gdb step count inside the symbol, callees excluded): plain +1, LF +6, CRLF +12, one backslash +7, lone CR +13, empty +2 (184 / 209 / 269 / 244 / 267 / 114 on main). UTF-16: +3 / +10 / +15 / +8 / +16 / +2. No flag read is on the paths without CR or LF. Those deltas are register and layout changes.
  • Passing toBe + toEqual: 4,224.4 instructions per pair on main, 4,222.3 here (user instructions from perf_event_open, slope between 1e6 and 2e6 pairs, run-to-run spread 2.5).
  • Passing Bun-format snapshot of 1,000 strings (a third each plain, LF, CRLF): 2,434.3 to 2,440.6 instructions per string (+6.3, A/A spread 0.1).
  • Failing toEqual with a visible difference and no \r: one-line strings 33,561 to 33,678 per message (+118, the two \r searches of the diff printer). Two LF strings 34,423 to 34,753 (+331, also one more \n search per multi-line string in the arm).
  • Release binary (llvm-size -A): .text 110,640,793 to 110,641,817 (+1,024), .rodata +0, stripped file 140,482,848 to 140,482,848 (+0). Tag::String arm 5,010 to 5,033 bytes. The fmt of DiffFormatter, which holds print_diff_main, 5,461 to 5,619. New push_with_visible_carriage_returns 164.
  • size_of::<pretty_format::Formatter>: 304 to 304 bytes (gdb, debug builds).
  • Stored snapshot text: 0 of 3 generated snapshot files differ (11,000 values with \r, \n, \r\n, backslashes and quotes in strings, keys, Map, Set, DOM nodes, Error, Symbol, with and without a serializer, 573,873 bytes, written by main and by this PR). The PR build reads the main-written files: 11,000 of 11,000 pass.
  • Snapshots written by bun 1.4.3 (a new corpus of 8,800 values, in this comment): 8,400 pass on main 3bf1a06 and here, and the same 400 fail on both. The 400 are all the String objects with a character above U+00FF, such as new String("a€b"). No other value fails. Bun 1.4.3 printed bytes of the UTF-16 data for them ("1": "\x00", "2": "¬"). 9b4284f (Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021, in 1.4.4) prints the code units, so 1.4.4 fails the same 400. Main and this PR write the same 804,988 bytes for this corpus. An earlier text of this line gave 96 of 8,500 from a corpus that was not kept.
  • CR repros with an empty diff: 10 to 0 of 17, without colours, with colours and in agent mode. Failure messages without a \r: 0 differing bytes (the two diffexample fixtures, with and without colours).
  • Rust allocator calls per failing toEqual message (gdb hit counts, debug builds, slope between 10,000 and 20,000 messages): 16 to 16 for a visible difference without \r, 15 to 16 when only the line ending differs. Getter runs per side: 2 to 2.
  • 0x0D bytes in the output of three failing tests (CRLF against LF, "10%\r20%" against "10%\r30%", "ab\r" against "ab"): stderr 0 to 0, GitHub annotation 0 to 0, junit.xml 0 to 0. ␍ in each: 4, 4 and 8. Main shows the first pair as equal everywhere.
  • Two other shapes of the string arm were measured and not taken. Choosing both search sets from the flag costs +47 instructions per snapshot string. Skipping the second search when the first terminator is \n adds a condition and costs +7.0 per snapshot string.

Tests

  • diffexample.test.ts: 21 failing assertions run in a child process, without colours, with colours and in agent mode. Each message is compared byte for byte.
  • expect-extend.test.js: this.utils.diff of two objects and of two strings. dom-nodes.test.ts: text, comment and attribute rows, and a snapshot file that main wrote for a node with \r.
  • existing-snapshots.test.ts: a snapshot that bun 1.4.2 wrote for strings with \r still passes.
  • Each clause was removed once in a debug build: without the field in did_older_bun_print the 1.4.2 snapshot fails, with \r kept in a stored DOM snapshot the snapshot file fails (the inline snapshot of the same node still passes), and without the line-break condition the stringContaining and array rows fail. With the \r rewritten before the diff (the first commit of this PR), the row of a literal ␍ prints the empty diff and fails.
  • Also run with the debug build: expect.test.js, vitest-apis.test.ts, spyMatchers.test.ts, mock-fn.test.js, expect-deferred.test.ts, jest-extended.test.js, test-test.test.ts, bun_test.test.ts, vitest-flavor.test.ts, dom-nodes-jest.test.js, test/regression/issue/18028.test.ts, test/js/bun/test/expect/, test/js/bun/test/snapshot-tests/, test/js/junit-reporter/junit.test.js, test/cli/test/bun-test.test.ts. In the snapshot suite, "error snapshots" fails with and without this change when stderr is not a terminal.
  • packages/bun-vscode parses these messages (processErrorData). No row holds a raw \r now. This is from a read of the code, not from a run.

[human-review] gate passed · iteration 0 · 9 files touched

fails on main (without fix) 
ASAN without fix: 5 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/expect-extend.test.js test/js/bun/test/printing/diffexample.test.ts test/js/bun/test/printing/dom-nodes.test.ts test/js/bun/test/snapshot-tests/existing-snapshots.test.ts
bun test v1.4.4 (367d939d9)

test/js/bun/test/expect-extend.test.js:
(pass) is available globally when matcher is unary [5.70ms]
(pass) is available globally when matcher is variadic [3.47ms]
(pass) exposes matcherUtils in context [1.51ms]
(pass) is ok if there is no message specified [2.44ms]
(pass) works with empty matcher name [0.94ms]
(pass) exposes an equality function to custom matchers [2.49ms]
(pass) defines asymmetric unary matchers [5.01ms]
(pass) defines asymmetric unary matchers that can be prefixed by not [3.02ms]
(pass) defines asymmetric variadic matchers [3.08ms]
(pass) defines asymmetric variadic matchers that can be prefixed by not [3.07ms]
(pass) prints the Symbol into the error message [3.07ms]
(pass) allows overriding existing extension [2.39ms]
(pass) throws descriptive errors for invalid matchers [4.12ms]
(pass) invalid matcher i
... (truncated)

release without fix: 5 FAILED
bun test v1.4.4-canary.1 (ca055d257)

test/js/bun/test/expect-extend.test.js:
(pass) is available globally when matcher is unary [0.13ms]
(pass) is available globally when matcher is variadic [0.06ms]
(pass) exposes matcherUtils in context [0.02ms]
(pass) is ok if there is no message specified [0.02ms]
(pass) works with empty matcher name
(pass) exposes an equality function to custom matchers [2.57ms]
(pass) defines asymmetric unary matchers [3.15ms]
(pass) defines asymmetric unary matchers that can be prefixed by not [0.14ms]
(pass) defines asymmetric variadic matchers [0.11ms]
(pass) defines asymmetric variadic matchers that can be prefixed by not [0.16ms]
(pass) prints the Symbol into the error message [0.13ms]
(pass) allows overriding existing extension [0.13ms]
(pass) throws descriptive errors for invalid matchers [0.19ms]
(pass) invalid matcher implementations errors > handles correctly when matcher throws [0.10ms]
(pass) invalid matcher implementations errors > throws when returns undefined [0.15ms]
(pass) invalid matcher implementations errors > throws when returns not an object [0.11ms]
(pass) invalid matcher implementations errors > throws when return is m
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/expect-extend.test.js test/js/bun/test/printing/diffexample.test.ts test/js/bun/test/printing/dom-nodes.test.ts test/js/bun/test/snapshot-tests/existing-snapshots.test.ts
bun test v1.4.4 (367d939d9)

test/js/bun/test/expect-extend.test.js:
(pass) is available globally when matcher is unary [4.98ms]
(pass) is available globally when matcher is variadic [3.34ms]
(pass) exposes matcherUtils in context [1.46ms]
(pass) is ok if there is no message specified [2.44ms]
(pass) works with empty matcher name [0.96ms]
(pass) exposes an equality function to custom matchers [2.45ms]
(pass) defines asymmetric unary matchers [4.76ms]
(pass) defines asymmetric unary matchers that can be prefixed by not [2.96ms]
(pass) defines asymmetric variadic matchers [3.00ms]
(pass) defines asymmetric variadic matchers that can be prefixed by not [3.14ms]
(pass) prints the Symbol into the error message [2.96ms]
(pass) allows overriding existing extension [2.34ms]
(pass) throws descriptive errors for invalid matchers [4.06ms]
(pass) invalid matcher i
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 618ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/353] gen generated_host_exports.rs
generated_host_exports.rs: 119 exports (lazy=4, generic=115, rust=0); 288 extern-C blocks audited
[2/352] gen cpp.rs (cppbind)
[3/352] gen the table of built-ins
builtins table: operands without uses: none
builtins table: 2101 host entries have a result that is not described; a program in which a value may be anything holds what any host class needs: 0 modules, 15 functions
[4/339] gen .bind.ts → Generated*.{cpp,h}, generated_bindgen_*.rs
[5/331] gen JS modules (bundle-modules)
Preprocess modules (545ms)
Bundle modules (112ms)
Postprocesss modules (1074ms)
Bundle Functions (193ms)
Generate Code (342ms)

[2.28s] Bundled "src/js" for production
  2776 kb
  209 internal modules
  13 native modules
  55 internal functions across 19 files
[6/85] cxx obj/unified/UnifiedSource-src_jsc_modules-0.cpp.o
[7/85] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-2.cpp.o
[8/85] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-0.cpp.o
[9/85] cxx obj/unified/UnifiedSo
... (truncated)
diff hotspot 
src/runtime/test_runner/diff/printDiff.rs          |  27 +++++
 src/runtime/test_runner/dom_format.rs              |   4 +-
 src/runtime/test_runner/pretty_format.rs           |  21 +++-
 test/js/bun/test/expect-extend.test.js             |   6 ++
 .../printing/__snapshots__/dom-nodes.test.ts.snap  |  16 +++
 test/js/bun/test/printing/diffexample.test.ts      | 113 ++++++++++++++++++++-
 test/js/bun/test/printing/dom-nodes.test.ts        |  21 ++++
 .../__snapshots__/existing-snapshots.test.ts.snap  |  14 +++
 .../test/snapshot-tests/existing-snapshots.test.ts |  11 ++
 9 files changed, 226 insertions(+), 7 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file 
file                                                      reads  edits  tests
src/runtime/test_runner/diff/printDiff.rs                     3      3     60
src/runtime/test_runner/dom_format.rs                         1      1     60
src/runtime/test_runner/pretty_format.rs                     11      7     62
test/js/bun/test/expect-extend.test.js                        0      0     24
…/bun/test/printing/__snapshots__/dom-nodes.test.ts.snap      0      0     61
test/js/bun/test/printing/diffexample.test.ts                 1      0     44
test/js/bun/test/printing/dom-nodes.test.ts                   2      3     27
…hot-tests/__snapshots__/existing-snapshots.test.ts.snap      0      0     61
…t/js/bun/test/snapshot-tests/existing-snapshots.test.ts      0      0     24

@robobun
bun:test: keep carriage returns in failure messages and show them as ␍
51ba862 
The value printer wrote "\r\n" and a lone "\r" in a string as "\n". That is
jest-snapshot's normalizeNewlines, for the text that a snapshot stores. A
failing matcher prints both values with the same printer, so two strings
that differ only in their line endings printed the same text and the diff
was empty.

Formatter.normalize_newlines is off by default. Only the two producers of
stored Bun-format snapshot text set it. The quoted-string arm and the DOM
printer read it, so a failure message keeps each "\r".

print_diff_main shows every "\r" of either side as U+240D. A terminal draws
what follows a raw "\r" over the start of the row, and a GitHub Actions
annotation drops it.
@github-actions github-actions Bot added the claude label Nov 3, 2026
@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. CI is red only on tests that are also red on main.

How it was reproduced

  • expect("a\r\nb").toBe("a\nb") and expect({ s: "a\r\nb" }).toEqual({ s: "a\nb" }) on main (271b65e, canary 1.4.4-canary.1+271b65ec6, linux x64). Both print the value once, then - Expected - 0 / + Received + 0.
  • Bun 1.2.19 prints Received: "a\r\nb" for the first assertion. Jest 30.5.2 and Vitest 5.0.5 print a changed row.
  • bun bd test test/js/bun/test/printing/diffexample.test.ts test/js/bun/test/expect-extend.test.js test/js/bun/test/printing/dom-nodes.test.ts: 5 tests fail on a debug build of main, and all pass on this branch.

CI

  • Build 137137 (38fad2d) is complete. Each red test is in test/aot/, or is test/bundler/compile-aot-size.test.ts. Main is red on the same tests (build 137130). This PR does not change that code.
  • No test under test/js/bun/test/ is red. The four test files of this PR pass on the first attempt on the ASAN, Windows and macOS lanes.

PR: #47816

@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 10:37 PM PT - Nov 2nd, 2026

❌ @robobun, your commit 38fad2d has 41 failures in Build #137137 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47816

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

bun-47816 --bun

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

Formatter and snapshot paths now use an explicit newline-normalization setting. Diff output displays carriage returns as ␍ without changing the bytes used for comparisons. Tests cover carriage returns in strings, snapshots, DOM nodes, and diagnostic output.

Changes

Carriage Return Formatting

Layer / File(s) Summary
Formatter and snapshot newline handling
src/runtime/test_runner/pretty_format.rs, src/runtime/test_runner/dom_format.rs, test/js/bun/test/printing/dom-nodes.test.ts, test/js/bun/test/snapshot-tests/existing-snapshots.test.ts		 Formatter now has a normalize_newlines setting. Snapshot formatting enables normalization for non-pretty output and older-Bun comparisons. DOM snapshot formatting preserves carriage returns when normalization is disabled. Tests cover carriage returns in snapshot values and DOM nodes.
Visible carriage returns in diffs
src/runtime/test_runner/diff/printDiff.rs, test/js/bun/test/expect-extend.test.js, test/js/bun/test/printing/diffexample.test.ts, test/js/bun/test/printing/dom-nodes.test.ts		 Diff rendering displays carriage returns as ␍ in expected and received values, while comparisons use the original bytes. Tests cover CRLF and lone-CR differences, matching CRLF values, DOM-node diffs, and diagnostic output modes.

Possibly related PRs

  • oven-sh/bun#37764: Both changes involve diff rendering; this change makes carriage-return differences visible to that logic.
  • oven-sh/bun#46729: Both changes involve pretty_format.rs and formatter behavior.

Suggested reviewers: alii, jarred-sumner

Merge Risk: Low · up to 38fad

Existing Bun-format snapshots using serializers with carriage-return data can unexpectedly fail. The regression is narrowly scoped; propagate newline normalization through serializer formatting to preserve compatibility.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: displaying carriage returns as ␍ in failing matcher diffs.
Description check Passed The description is thorough and follows the template. It has a 'What does this PR do?' equivalent (Problem, Fix, Background, Downsides, and detailed notes) and a 'How did you verify your code works?' …
Full details: Description check

Explanation

The description is thorough and follows the template. It has a 'What does this PR do?' equivalent (Problem, Fix, Background, Downsides, and detailed notes) and a 'How did you verify your code works?' equivalent (Tests and Measurements, plus the gate evidence with fail-on-main and pass-on-PR runs). It is long, and the required headings are present under different names. The author describes the Problem, Fix, and verification steps clearly and lists downsides and open PRs. Nothing critical is missing.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/python/security/crewai-code-execution-python.yml

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between ca055d2 and 38fad2d.

Files selected for processing: 5
  • src/runtime/test_runner/diff/printDiff.rs
  • src/runtime/test_runner/pretty_format.rs
  • test/js/bun/test/printing/diffexample.test.ts
  • test/js/bun/test/printing/dom-nodes.test.ts
  • test/js/bun/test/snapshot-tests/existing-snapshots.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

coderabbitai[bot]
coderabbitai Bot reviewed Nov 3, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread src/runtime/test_runner/diff/printDiff.rs Resolved
claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the stored-snapshot producers: both print_snapshot branches and did_older_bun_print set normalize_newlines, the has_newline rewrite short-circuits on that flag so the Bun-format text path is unchanged, and the two added .snap files contain only \n. The .not early return in print_diff_main receives the ␍-rewritten expected slice, so that path is covered too.

Extended reasoning...

The change touches the bun:test value printer (pretty_format.rs string arm), the DOM printer flag source, and print_diff_main, adding a per-formatter normalize_newlines flag and a CR-to-U+240D rewrite of both diff inputs; it touches no security-sensitive surface. Two confirmed findings are posted inline (U+240D collision in the diff and raw CR still reaching non-diff DOM messages), so a human look is already signalled; this note only records the snapshot-storage and .not paths that were examined and found consistent.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/runtime/test_runner/dom_format.rs — Users whose DOM-node matcher fails without a diff still get a raw \r in the failure message, which the terminal overdraws and the GitHub Actions annotation drops. The PR's own goal is that a \r never reaches the terminal, yet print_in_message at dom_format.rs:35 keeps keeps_carriage_returns: true and dom_format.rs:699/:712 then copy \r from text, comments and attribute values straight into the message for every matcher built on make_formatter (mod.rs:258-263), none of which pass through print_diff_main. Fix: route the same ␍ rendering through the message printer for DOM nodes (a non-diff message sibling of print_diff_main), or rewrite \r in the ConsoleObject-based message path, so every producer of failure text is covered.

    Why this was flagged

    Trigger: a DOM node whose text, comment or attribute contains \r, failing a matcher that prints without a diff: toBeInstanceOf (expect/toBeInstanceOf.rs:25,41), toContain (toContain.rs:118), toBeEmpty (toBeEmpty.rs:73-120) and the list rendering of the CalledWith family (toHaveBeenCalledWith.rs:100). Those use make_formatter at mod.rs:258-263, which installs dom_format::print_in_message as the dom_printer; print_in_message builds its Printer with keeps_carriage_returns: true at dom_format.rs:35, so write_text at :699 uses the needle <> and write_quoted at :712 (min is true) uses "\\, neither of which touches \r, and the raw byte lands in the message. The base branch does the same, but this PR adds with_visible_carriage_returns only to print_diff_main (printDiff.rs:39, :54) and states that a raw \r must not reach the terminal or an annotation. Population: tests of DOM output containing CRLF text using a non-diff matcher; rate: every such failure. Remedy: apply the ␍ rewrite in the message printer for DOM nodes as well, or in the shared ConsoleObject message path.

    Verification: pre-existing. Trigger: a happy-dom/jsdom node whose text, comment or attribute value contains a \r, failing a matcher that prints the value without going through print_diff_main. src/runtime/test_runner/dom_format.rs:35 still constructs the Printer with keeps_carriage_returns: true, so a \r is copied verbatim into the message. The base commit already has the same line, so merging makes nothing worse.

Comment thread src/runtime/test_runner/diff/printDiff.rs Resolved
@robobun
bun:test: draw a carriage return as ␍ when a diff row is written, not…
ca055d2 
… before the diff

The rewrite ran on both sides before the diff, so a "\r" and a literal
U+240D became equal bytes. A multi-line pair that differs only there got
the empty diff again.

The diff now compares the bytes as they are. Printer::b, which writes the
text of a value, draws each "\r" as U+240D. A "\r" that is the only change
keeps the style of a whitespace-only change.
github-actions[bot]
github-actions Bot reviewed Nov 3, 2026
View reviewed changes
Comment thread src/runtime/test_runner/diff/printDiff.rs Resolved
github-actions[bot]
github-actions Bot reviewed Nov 3, 2026
View reviewed changes
Comment thread src/runtime/test_runner/pretty_format.rs Resolved
@robobun

robobun commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

Both reviews found the same defect in printDiff.rs. The \r was rewritten to ␍ before the diff ran, so a \r and a literal ␍ became equal bytes. A multi-line pair that differs only there got the empty 0/0 diff again.

ca055d2 moves the drawing to Printer::b, which writes the text of a row. The diff now compares the bytes as they are.

  • expect("a\r\nb").toBe("a␍\nb") prints - "a␍ / + "a␍ / b" with a 1/1 footer. A new test row pins it. On the first commit that row prints the empty diff.
  • With colours, the \r has the inverse style of a whitespace-only change, and the ␍ of the value has the colour of its row.
  • Without colours, the one-line form still draws both values the same: Expected: "a␍" / Received: "a␍". An escape for the literal ␍ moves the collision to another pair, because this printer does not escape its own escape character. The PR body lists this under Downsides.

The raw \r from print_in_message (a DOM node in a message that has no diff) is the same on main. this.utils.stringify() returns that text too, so this PR does not change it. The PR body lists it under "What is not in this PR".

claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline nit, I checked a few more things since the last push: the U+240D substitution now happens only at draw time in Printer::b / the .not path, so a literal ␍ against a \r still diffs on raw bytes and produces marked rows with a 1/1 footer (pinned by the againstALiteral case). Every value-text write in printDiff.rs goes through segment -> b (including truncated_line and the char-highlight path), so no path emits a raw \r. normalize_newlines is set on both Bun-format snapshot producers (print_snapshot, did_older_bun_print) and read by the string arm and dom_format, and the keeps_carriage_returns remap is equivalent to the old is_pretty_format() for Jest/Vitest, changing only failure-message output.

Extended reasoning...

The change touches the bun:test value printer, DOM printer and diff renderer so that failure messages keep carriage returns and draw them as U+240D while stored Bun-format snapshot text is unchanged; no security-sensitive surface is involved. The previously raised ambiguity concern was addressed by moving the substitution after the byte-level diff, and the new subprocess tests assert exact output under three env configurations. A human look is still worthwhile because this is a user-visible change to failure output across many matchers and the description names several open PRs editing the same lines.

Comment thread test/js/bun/test/printing/diffexample.test.ts Resolved
coderabbitai[bot]
coderabbitai Bot reviewed Nov 3, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread src/runtime/test_runner/pretty_format.rs Resolved
claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the latest push and found no bugs; the two points raised on earlier versions are addressed in the code (the diff now compares raw bytes and only Printer::b draws ␍, and the test comment is one line), but since this changes user-visible failure-message output for every string with a \r, a human look at the design choice is still worthwhile.

What was reviewed:

  • Traced normalize_newlines through every Formatter constructor: only did_older_bun_print and the Bun-format branch of print_snapshot set it, so stored snapshot text is unchanged; dom_format.rs now keys off the same field.
  • All value text in printDiff.rs flows through b (segment, truncated_line); the .not branch rewrites into a local buffer. Byte-search goes through bun_core::strings, no str::find/memchr.
  • One behavioural change worth a maintainer's opinion rather than a bug: matchers that print without a diff via make_formatter (toContain, toMatch, toHaveLength, etc.) now emit a raw \r in Received: where main printed \n; the PR notes github_action_writer drops a lone \r, and no test pins that path.
Extended reasoning...

The change touches the bun:test value printer (pretty_format.rs), the DOM printer and the diff renderer (printDiff.rs), about 50 lines of Rust, plus exact-message tests under FORCE_COLOR=0/1 and AGENT=1 and new snapshot cases. It touches no security-sensitive surface. The flag defaults off and is set only on the two snapshot-text producers, and the ␍ substitution is applied at output time after the byte diff, which closed the ambiguity raised on an earlier push. Deferring rather than approving because the PR changes user-visible failure output across all matchers, including a raw \r now reaching non-diff messages from make_formatter that no test covers, which is a design choice a maintainer should weigh; no CODEOWNERS entry covers these files and there is no outstanding CHANGES_REQUESTED review.

@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Two points came up in the reviews of 38fad2d. Both were checked against main 6a38049. Neither needs a change.

Serializer output in a Bun-format snapshot (the thread on pretty_format.rs:320)

  • The stored text does not change. print_for_serializer prints through print_like_pretty_format, which copies a string as it is. It does not reach the string arm that reads normalize_newlines, and main keeps the \r there too.
  • 7 snapshots of values with \r in a file with a serializer that calls printer(), and 2,500 generated ones, have the same sha256 on main and on this branch.
  • CodeRabbit withdrew the finding in that thread. Its summary comment still carries the merge risk text of that finding.

Messages without a diff (toContain, toMatch, toBeNull and the like)

  • These messages do not change. make_formatter (src/runtime/test_runner/mod.rs:258) builds bun_jsc::console_object::Formatter, the formatter of console.log. It is not the snapshot printer, so it does not read normalize_newlines.
  • It writes a \r as the two characters \r, on main and here: Received: "a\rb".
  • Measured with 35 failing assertions on values that hold a \r. 33 messages have no diff, and they are byte-identical on main and on this branch, with and without colours. The other two hold a diff (toBe, toHaveBeenLastCalledWith). No message holds a raw \r byte on either build.
  • JestPrettyFormat::format, the snapshot printer in its default state, has two callers. Both are in DiffFormatter::new (diff_format.rs:34 and :43), and print_diff_main draws all of that text.

@robobun robobun changed the title bun:test: keep carriage returns in failure messages and show them as ␍ bun:test: show a carriage return as ␍ in the diff of a failing matcher Nov 3, 2026
@robobun

robobun commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

A correction to one measurement in the description.

The description said that 96 of 8,500 snapshots written by bun 1.4.3 fail on main and on this branch. It did not say which values they are, and the corpus of that run was not kept. I generated a new corpus, ran it again, and replaced that line.

Result (8,800 values, CI=true bun test, linux x64)

.snap written by Read by the writer Read by 1.4.4 Read by main 3bf1a06 Read by this PR (38fad2d)
1.4.2 or 1.4.3 (the same bytes) 8,800 pass 8,400 pass, 400 fail 8,400 pass, 400 fail 8,400 pass, 400 fail
1.4.4 8,800 pass 8,800 pass 8,800 pass 8,800 pass
main or this PR (the same 804,988 bytes) 8,800 pass not run 8,800 pass 8,800 pass
  • The 400 are the same values for each reader. Each one is a String object with a character above U+00FF, for example new String("a€b"). All 400 such values in the corpus fail. No other value fails.
  • Bun 1.4.2 and 1.4.3 printed the first bytes of the UTF-16 data of such a String object, one byte per index: "0": "a", "1": "\x00", "2": "¬". 9b4284f (Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021, released in 1.4.4) prints the code units: "0": "a", "1": "€", "2": "b". The 1.4.3 and 1.4.4 files differ in these 400 entries only.
  • This PR does not change which of these snapshots pass.

The corpus

  • 44 kinds of values: strings, String objects, keys, Map, Set, Error, Symbol, and stand-ins for DOM nodes and collections.
  • For each kind, 100 texts drawn from \r, \n, \r\n, a backslash, three quotes and filler, and 100 texts that also hold a character above U+00FF.
  • main is the canary build of 3bf1a06. This PR is a debug build of 38fad2d.
older.test.ts 
// Which snapshots of an older Bun still pass? Run once with the older Bun to write the file, then with CI=true and the newer one.
// Every value holds a drawn text of \r, \n, \r\n, backslashes, quotes and filler. Half of the texts also hold a 16-bit character.
import { expect, test } from "bun:test";

const perKind = Number(process.env.PER_KIND ?? 100);

// No Math.random: every binary must see the same values.
function rng(seed: number) {
  let a = seed >>> 0;
  return () => {
    a = (a + 0x6d2b79f5) >>> 0;
    let t = a;
    t = Math.imul(t ^ (t >>> 15), t | 1);
    t ^= t + Math.imul(t ^ (t >>> 7), t | 61);
    return ((t ^ (t >>> 14)) >>> 0) / 4294967296;
  };
}
function hash(text: string) {
  let h = 2166136261;
  for (let i = 0; i < text.length; i++) h = Math.imul(h ^ text.charCodeAt(i), 16777619);
  return h >>> 0;
}

const LATIN1 = ["\r", "\n", "\r\n", "\\", '"', "'", "`", "a", "b", " "];
const WIDE = ["\u20ac", "\u2028", "\u240d", "\ud83d\ude00", "\ud800"];
function draw(random: () => number, wide: boolean) {
  const signs = wide ? [...LATIN1, ...WIDE] : LATIN1;
  const parts: string[] = [];
  const length = 1 + Math.floor(random() * 6);
  for (let i = 0; i < length; i++) parts.push(signs[Math.floor(random() * signs.length)]);
  if (wide && !parts.some(part => WIDE.includes(part))) {
    parts[Math.floor(random() * parts.length)] = WIDE[Math.floor(random() * WIDE.length)];
  }
  return parts.join("");
}

// Stand-ins for DOM nodes, shaped like the ones in test/js/bun/test/printing/dom-nodes.test.ts.
const impl = Symbol("impl");
class ArrayLike {
  [index: number]: unknown;
  constructor(items: unknown[]) {
    items.forEach((item, i) => (this[i] = item));
  }
  get length() {
    return Object.keys(this).length;
  }
  *[Symbol.iterator]() {
    for (let i = 0; i < this.length; i++) yield this[i];
  }
}
class NodeList extends ArrayLike {}
class HTMLCollection extends ArrayLike {}
class NamedNodeMap extends ArrayLike {}
class DOMStringMap {}
class DOMTokenList {}
class Attr {
  constructor(
    public name: string,
    public value: unknown,
  ) {}
}
class Node {
  constructor(state: object) {
    Object.defineProperty(this, impl, { value: state });
  }
  get nodeType() {
    return (this as any)[impl].nodeType;
  }
  get data() {
    return (this as any)[impl].data;
  }
  get tagName() {
    return (this as any)[impl].tagName;
  }
  get attributes() {
    return new NamedNodeMap((this as any)[impl].attributes);
  }
  get childNodes() {
    return new NodeList((this as any)[impl].childNodes);
  }
  hasAttribute(name: string) {
    return (this as any)[impl].attributes.some((attribute: Attr) => attribute.name === name);
  }
  get nodeName() {
    return (this as any)[impl].tagName ?? "#node";
  }
}
class Text extends Node {}
class Comment extends Node {}
class DocumentFragment extends Node {}
class Element extends Node {}
class HTMLElement extends Element {}
const text = (data: string) => new Text({ nodeType: 3, data });
const comment = (data: string) => new Comment({ nodeType: 8, data });
const fragment = (...childNodes: unknown[]) => new DocumentFragment({ nodeType: 11, childNodes });
const h = (tagName: string, attributes: Record<string, unknown> = {}, ...children: unknown[]) =>
  new HTMLElement({
    nodeType: 1,
    tagName: tagName.toUpperCase(),
    attributes: Object.entries(attributes).map(([name, value]) => new Attr(name, value)),
    childNodes: children.map(child => (typeof child === "string" ? text(child) : child)),
  });

class Named {
  constructor(public value: unknown) {}
}
class Shout extends String {}

const VALUES: Record<string, (s: string) => unknown> = {
  "string": s => s,
  "string in an object": s => ({ k: s }),
  "string in an array": s => [s],
  "string nested": s => ({ a: [{ b: s }, s], c: { d: s } }),
  "string in a class instance": s => new Named(s),

  "String object": s => new String(s),
  "String object in an array": s => [new String(s)],
  "String object in an object": s => ({ k: new String(s) }),
  "object of a String subclass": s => new Shout(s),

  "key": s => ({ [s]: 1 }),
  "key nested": s => ({ a: { [s]: [s] } }),
  "key of a class instance": s => Object.assign(new Named(1), { [s]: 2 }),
  "Symbol key": s => ({ [Symbol(s)]: 1 }),

  "Map key": s => new Map([[s, 1]]),
  "Map value": s => new Map([["k", s]]),
  "Map nested": s => ({ m: new Map([[s, { [s]: s }]]) }),

  "Set": s => new Set([s]),
  "Set nested": s => [new Set([s, [s]])],

  "Error": s => new Error(s),
  "Error in an array": s => [new Error(s)],
  "Error in an object": s => ({ e: new TypeError(s) }),
  "Error with a property": s => Object.assign(new Error(s), { code: s }),
  "Error with a cause": s => new Error("outer", { cause: new Error(s) }),
  "Error name": s => Object.assign(new Error("m"), { name: s }),

  "Symbol": s => Symbol(s),
  "Symbol in an array": s => [Symbol(s)],
  "Symbol in an object": s => ({ k: Symbol(s) }),

  "DOM text": s => text(s),
  "DOM comment": s => comment(s),
  "DOM attribute": s => h("div", { title: s }),
  "DOM child text": s => h("p", { id: "x" }, s),
  "DOM nested": s => h("div", { class: "a" }, h("span", { title: s }, s), comment(s)),
  "DOM fragment": s => fragment(text(s), h("b", {}, s)),
  "DOM node in an object": s => ({ node: h("i", { title: s }, s) }),
  "DOM node in an array": s => [h("i", { title: s }, s), text(s)],
  "DOM node in a Map": s => new Map([[s, h("i", { title: s }, s)]]),
  "DOM node in a Set": s => new Set([h("i", { title: s }, s)]),
  "DOM node with ownerDocument": s => Object.assign(h("i", { title: s }, s), { ownerDocument: { title: s } }),
  "NodeList": s => new NodeList([text(s), h("b", {}, s), s]),
  "HTMLCollection": s => new HTMLCollection([h("a", { href: s })]),
  "NamedNodeMap": s => new NamedNodeMap([new Attr("title", s), new Attr("id", "x")]),
  "DOMStringMap": s => Object.assign(new DOMStringMap(), { k: s, [s]: "v" }),
  "DOMTokenList": s => Object.assign(new DOMTokenList(), [s, "y"]),
};

function each(kind: string, run: (s: string) => void) {
  for (const wide of [false, true]) {
    const random = rng(hash(kind) ^ Number(wide));
    for (let i = 0; i < perKind; i++) {
      const s = draw(random, wide);
      test(`${kind} | ${wide ? "16-bit" : "Latin-1"} | ${i}`, () => run(s));
    }
  }
}

for (const [kind, make] of Object.entries(VALUES)) {
  each(kind, s => {
    expect(make(s)).toMatchSnapshot();
  });
}
each("thrown Error", s => {
  expect(() => {
    throw new Error(s);
  }).toThrowErrorMatchingSnapshot();
});

Run it once with the older release in an empty directory. That writes __snapshots__/older.test.ts.snap. Then run CI=true bun test ./older.test.ts with the newer build.

@robobun robobun mentioned this pull request Nov 5, 2026
Open
This was referenced Nov 8, 2026
Open
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@claude claude[bot] claude[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun