console/test: serialize DOM nodes as markup in Bun.inspect and matcher utils - #35712
console/test: serialize DOM nodes as markup in Bun.inspect and matcher utils#35712robobun wants to merge 11 commits into
Conversation
…r utils
Adds a DOMNode tag to ConsoleObject::Formatter and the test-runner
pretty-format so objects from jsdom / happy-dom whose constructor name
matches the pretty-format DOMElement plugin pattern (HTML*/SVG*/Element,
Text, Comment, DocumentFragment) and whose nodeType agrees are printed
as markup (<button id="x">text</button>) instead of having their entire
object graph dumped.
This covers:
- console.log / Bun.inspect
- this.utils.printReceived / printExpected / stringify in custom matchers
- toMatchSnapshot / toMatchInlineSnapshot
Detection bails out in two pointer reads for plain {} / Object.create(null)
so the common path does not pay for a calculatedClassName walk.
Fixes #10886
WalkthroughChangesDOM-like objects are identified through prototype and DOM node inspection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 97f8ce9 has some failures in 🧪 To try this PR locally: bunx bun-pr 35712That installs a local version of the PR into your bun-35712 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ConsoleObject.rs`:
- Around line 3346-3387: Update is_dom_node so custom-element detection walks
upward through multiple prototype levels, rather than checking only grand.
Continue until a recognized DOM class name is found, using a bounded traversal
to avoid unbounded chains; preserve the existing nodeType confirmation and false
result when no recognized prototype is found.
In `@src/runtime/test_runner/pretty_format.rs`:
- Around line 1214-1235: Update the attribute formatting loop in the relevant
pretty-format branch to enable multiline output only when pairs.len() > 1,
matching the console formatter’s attrs_multiline behavior. Keep single-attribute
elements inline as <button id="x" />, and ensure has_attrs represents whether
multiline attributes were emitted so the closing newline/indent logic remains
consistent.
- Around line 1258-1280: The child-formatting recursion in Formatter’s format
path lacks a depth limit. Add and thread a depth counter with a defined maximum,
and check it before iterating and recursively formatting child nodes; when the
limit is reached, truncate the nested output consistently with the existing
ConsoleObject behavior while preserving indentation and normal formatting below
the limit.
In `@test/js/bun/util/inspect.test.js`:
- Around line 808-834: Replace the lookalike DOM fixtures in the “DOM nodes”
tests with real nodes created by both jsdom and happy-dom, preserving coverage
for the relevant node and attribute shapes. Add regression assertions for
inspection/rendering through each library’s nodes and verify the console.log
formatting path as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e9d03bf-298b-4874-9bde-d20b792ce3bd
📒 Files selected for processing (3)
src/jsc/ConsoleObject.rssrc/runtime/test_runner/pretty_format.rstest/js/bun/util/inspect.test.js
|
Status: diff is green, waiting on a clean CI roll.
Pushed one empty retrigger (97f8ce9) to re-roll. All review threads are resolved. Ready for a maintainer. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs this pass. Because it adds ~500 lines of new native formatting logic across two formatters — a duck-type detection heuristic that now runs on every Object/FinalObject in Tag::get, plus user-visible output changes for Bun.inspect/console.log/snapshots — a human sign-off on the detection shape and the intentionally-deferred nits (Text/Comment/backslash escaping, unclamped .length iteration) would be worthwhile.
What was reviewed
is_dom_nodeprototype walk: bails on null grand-proto and Proxy, bounded to 8 hops,nodeTypegetter throw is swallowed viaclear_exception_except_termination(termination still propagates).Tag::DOMNodeadded tocan_have_circular_referencesin both formatters, so the visited-map cycle guard applies.print_dom_nodechild recursion:defer_decrement!restoresindent/depthon the?path in ConsoleObject; the pretty_format twin restoresindentvia theinner?closure pattern.- Negative tests confirm non-DOM class names and mismatched
nodeTypestill fall through toTag::Object.
Extended reasoning...
Overview
Adds a DOMNode tag to both ConsoleObject::Formatter (Bun.inspect / console.log / matcher-utils) and test_runner::pretty_format::Formatter (snapshots / toEqual diff). Detection is duck-typed: class-name pattern /^((HTML|SVG)\w*)?Element$|^Text$|^Comment$|^DocumentFragment$/ plus a matching nodeType, with a bounded 8-level prototype walk for custom elements. Rendering mirrors pretty-format's DOMElement plugin (sorted attributes, self-closing when empty, … at max_depth). ~300 lines in ConsoleObject.rs, ~200 in pretty_format.rs, ~180 lines of new tests.
Security risks
None material. This is a diagnostic/output path with no privileged effects. The new code reads user-controlled JS properties (nodeType, tagName, attributes[i].name/value, childNodes[i]), each of which can be a throwing/side-effecting getter — the implementation swallows those via clear_exception_except_termination and falls back, which is the established pattern for the formatter. The unclamped 0..n iteration over a JS-supplied .length is a self-DoS surface only (already discussed and consciously deferred by the author as matching pretty-format).
Level of scrutiny
Medium-high. Not memory-safety-sensitive in the usual sense (no raw pointers held across JS calls; all reads go through JSValue::get/get_index with owned Vec<u8> copies), but:
is_dom_nodenow runs insideTag::get_advanced/Tag::getfor everyObject/FinalObjectformatted — a per-value cost on a path REVIEW.md flags as perf-sensitive. The fast-bail was tightened after my earlier nit, but a maintainer should confirm thecalculatedClassNamecost is acceptable.- This is a user-visible API/output change: any object whose constructor happens to be named
Text,Comment,Element, orHTML*Element/SVG*Elementand has a matching integernodeTypewill now render as markup instead of an object dump. That's the point, but it's a behavior a maintainer should ratify. - Several review nits were explicitly left for follow-up (Text/Comment
</>escaping, lone-backslash attribute escaping, unclamped iteration). A human should confirm those deferrals are acceptable for merge.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved — either fixed (bounded ancestor walk, attr-quote escaping, single-level-class fast-bail) or explicitly deferred with a stated rationale. CI is green on the touched test file across all lanes per the author. Test coverage is solid for the happy path and includes negative cases (wrong nodeType, non-DOM class name, throwing getter), the matcher-utils path, and the snapshot path. The duck-typed fixtures were defended (and accepted by CodeRabbit) on debug+ASAN cost grounds. No outstanding unaddressed reviewer requests.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; all earlier inline threads are resolved. It's a ~500-line feature addition that changes user-visible Bun.inspect / console.log / matcher-utils / snapshot output and adds a duck-typed detection step to the per-object tag path, with a few escaping/format trade-offs intentionally deferred — worth a maintainer sign-off rather than an auto-approve.
Checked: is_dom_node fast-bail and bounded ancestor walk (single-level user classes now cost one calculatedClassName, not two); Tag::DOMNode participates in can_have_circular_references so the visited map short-circuits cycles in the snapshot serializer; clear_exception_except_termination on getter failures matches the existing pattern used elsewhere for formatter-side user-JS calls; depth cutoff and defer_decrement! restore indent/depth on the ConsoleObject path.
Extended reasoning...
Overview
Adds a DOMNode tag to both ConsoleObject::Formatter and the test-runner pretty_format so jsdom/happy-dom nodes render as markup instead of a full object-graph dump. Detection is duck-typed (class-name pattern + matching nodeType, with a bounded ancestor walk for custom elements); rendering handles Element/Text/Comment/DocumentFragment with sorted attributes, depth truncation (console path), and per-line attributes (snapshot path). ~300 lines in src/jsc/ConsoleObject.rs, ~200 in src/runtime/test_runner/pretty_format.rs, plus 15 new tests in inspect.test.js.
Security risks
None material. This is a diagnostic/formatting path; the only user-controlled inputs are JS objects already in-process. clear_exception_except_termination is used to swallow throws from user getters (nodeType, attributes[i], etc.), which matches how other formatter arms handle hostile getters and is an established helper in the codebase.
Level of scrutiny
Medium-high. It is not a mechanical fix: it changes what Bun.inspect, console.log, this.utils.printReceived/stringify, toEqual diffs, and toMatchSnapshot emit for a whole class of objects, and inserts a new check (is_dom_node) into Tag::get_advanced / Tag::get for every Object/FinalObject formatted. The heuristic, output layout, and the choice to lowercase tagName unconditionally (SVG tag names are case-sensitive in pretty-format's plugin) are design calls a maintainer should confirm.
Other factors
All prior inline threads are resolved. Three were intentionally deferred by the author with stated reasoning: (a) Text/Comment </> are not HTML-escaped, (b) lone \ in attribute values is not escaped (only " is), (c) attributes/childNodes loop bounds are not clamped. All are cosmetic or contrived and don't block, but they're conscious divergences from Jest's DOMElement plugin that a human should ack. Tests use hand-built lookalike fixtures rather than real jsdom/happy-dom (author verified manually against both); CI is green on the touched test file. Given the size, the user-visible behavior change, and the deferred trade-offs, deferring to a human reviewer rather than auto-approving.
|
One correction to the summary above, for whoever picks this up: lowercasing |
What
Bun.inspect,console.log,this.utils.printReceived/printExpected/stringify, andtoMatchSnapshotnow serialize DOM nodes (from jsdom / happy-dom) as markup instead of dumping the full object graph.instead of
Fixes #10886. Fixes #5540.
Not related to #29709 beyond the issue link: that PR fixes a native stack overflow when printing circular JSX / deep Proxy chains, while the #10886 report is a happy-dom element (a plain object to the formatter) producing a bounded but enormous dump, which is what this PR changes. Both still reproduce on current main; this branch has been merged with main.
Reproduction
Before this change the failure message from
toBeDisabled()printed the whole happy-domHTMLButtonElementtree (theownerDocument→windowgraph), which users describe as "infinite". Jest and Vitest print<button />becausethis.utils.printReceivedis backed bypretty-formatwith theDOMElementplugin.Cause
ExpectMatcherUtils.printReceived/stringifyroute throughConsoleObject::Formatter, which had no handling for DOM nodes (there is no native DOM in bun; these objects come from jsdom / happy-dom as plain JS class instances). The formatter already has aJSXtag for React elements but nothing forElement/Text/Comment/DocumentFragment, so they fell through toTag::Objectand were walked as ordinary objects. The snapshot serializer inpretty_format.rshad the same gap.Fix
Add a
DOMNodetag to bothConsoleObject::Formatterand the test-runnerpretty_formatthat mirrors thepretty-formatDOMElementplugin:Object/FinalObjectis a DOM node when its class name matches/^((HTML|SVG)\w*)?Element$/,Text,Comment, orDocumentFragmentand itsnodeTypeagrees with that class. The name check runs first sonodeType(a prototype getter) is never invoked on unrelated objects, and a two-pointer prototype probe bails out immediately for plain{}/Object.create(null)so the common path does not pay for acalculatedClassNamewalk.<tag attr="v">...children...</tag>(attributes sorted, self-closing when empty,…leaf atmax_depth);Textas itsdata,Commentas<!--data-->,DocumentFragmentas<DocumentFragment>…</DocumentFragment>.This matches what browsers do for
console.log(element)and what Jest does forprintReceived/ snapshots.util.inspectis unchanged (still Node-compatible).Verification
bun bd test test/js/bun/util/inspect.test.js -t "DOM nodes": 12 new tests covering HTML/SVG elements, Text/Comment/DocumentFragment, nested depth,printReceived/stringifyin a custom matcher, and the negative cases (wrongnodeType, non-DOM class name, throwingnodeTypegetter). All fail on current release and pass with this change.Checked against happy-dom 15/20 and jsdom.
no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js