Repository navigation
Conversation
A snapshot in Bun's format stored a Temporal value as its class name
alone, `PlainDate {}`, so the snapshot passed for any value.
`Formatter::print_as` now prints the text of
`SnapshotFormat__temporalText` in a snapshot too, not only in a
failure message: `Temporal.PlainDate 2020-01-02`.
An entry that an older Bun stored still passes. The replay of the
earlier text does not take the new branch, and the new print clears
`same_as_older_bun`.
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
|
Status Reproduced on import { test, expect } from "bun:test";
const day = process.env.DAY ?? "01";
test("PlainDate", () => {
expect(Temporal.PlainDate.from("2020-01-" + day)).toMatchSnapshot();
});
With this branch, step 1 stores PR: #48106, stacked on #48105. CI (build 138289): no failure of the build is in a test of this PR, and the tests of this PR pass on the lanes that ran them. 28 test files are red on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The test "and fails for another value, and for the string of the value" looked for `Received: Temporal.PlainDate 2020-01-02` in the thrown message. A run with colours puts escape codes inside that text.
…only The test "and fails for another value, and for the string of the value" held three inline snapshots that must not match. `bun test -u` on this file rewrites such a snapshot, and the test fails after that. The spawned tests of the same block already compare a value with the entry of another value and with the entry of its string. The check of the diff line of an object moves to one of them.
There was a problem hiding this comment.
I re-reviewed the latest push and found no bugs; the last commit moves the deliberately mismatching inline snapshots into the spawned fixture, which settles the nit I left earlier. A maintainer should still look, since the stored text for a Temporal value (Temporal.PlainDate 2020-01-02 versus the quoted toJSON() string Jest and Vitest store) is a user-facing format choice the PR itself leaves open.
What was reviewed:
- The new gate in
Formatter::print_as(src/runtime/test_runner/pretty_format.rs:1088-1095): it mirrors the DOM-node branch informat(skipped whenlike_older_bunis set, clearssame_as_older_bunotherwise), sodid_older_bun_printstill replaysPlainDate {}andexpect.rs:1355-1357runs that replay on a mismatch. - Which callers now reach the branch: only failure messages (which already printed Temporal values via
output_length) and Bun-formatprint_snapshot; Jest/Vitest formats and serializer plugins go throughprint_like_pretty_format, andformatchecks serializers beforeprint_as. print_temporalreads internal slots throughSnapshotFormat__temporalTextwith notoJSON/toStringcall; the spawned fixture asserts 0 of 16 spies fire.
Extended reasoning...
The production change is five lines in src/runtime/test_runner/pretty_format.rs that widen the Temporal fast path from failure messages to Bun-format snapshots and mark such output as differing from older Bun so legacy PlainDate {} entries keep passing; the rest is ~290 lines of tests and two docs paragraphs. It touches no security-sensitive surface. The code mirrors an existing pattern and the tests cover each class, nesting, older-Bun entries, --update-snapshots, polyfills and serializers, but the snapshot text format is a user-visible decision the author explicitly says no maintainer has made, and my two earlier optional findings were closed by the author without code changes, so a human decision is still needed rather than an approval.
Before merge (Notes): a yes on the stored text, #48105, the release with #44833, #46729.
Problem
PlainDate {}. The test still passes after the date changes.PlainDate {}#48105 prints the value in failure messages only. Its branch inFormatter::print_as(src/runtime/test_runner/pretty_format.rs:1087) skips a snapshot.Fix
Temporal.PlainDate 2020-01-02, read from the slots with notoJSONcall.PlainDate {}still passes, for any value.bun test -urewrites it.PlainDateTime.toJSON().test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts(7 new tests, 6 fail on bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105). Also the snapshot suites.Background
toJSON()string,"2020-01-02", and bun test: snapshots don't serialize values through toJSON() like Jest and Vitest #46043 asks for it here. A date and its string then store the same bytes.Downsides
Temporal.Nowvalue fails on each run after-u. The docs name two remedies.Bun.deepEqualsignores both.ZonedDateTime+1,735. A storedPlainDate {}entry prints twice.Notes
Before merge
Temporal.PlainDate 2020-01-02. Two values store one text only iftoEqualcalls them equal. AZonedDateTimeisTemporal.ZonedDateTime 2020-01-01T10:00:00Z[Pacific/Honolulu].toString(). The same text but for 4 cases (the table in bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105). ItsZonedDateTimetext changes with the time zone data: 6 of 2,670 probed pairs of zone and instant differ between ICU 78.3 and ICU 79.1.toJSON()string,"2020-01-02", as Jest 30.5.2 and Vitest 5.1.0 store it. Bun stores it under a Jest or Vitest header. That text is also what the string"2020-01-02"stores.toJSON()of aPlainDateTimein the pinned JavaScriptCore drops the calendar (Node 26.3.0 keeps it), so two values thattoEqualtells apart store one text. A value from a Temporal polyfill gets a text fromtoJSON()and not from this PR.test.eachrow is part of the same question. A title is the key of the entry. It prints through theconsole.logprinter: the key of"opens on $v"for aPlainDateis 792 characters of prototype members. This PR does not change it.PlainDate {}#48105. This PR is stacked on it. The diff here is 5 lines of Rust, the tests and the docs.PlainDate {}beside the new text of an accessor, a non-enumerable property, a DOM node, a React 19 element, a mock function, a native constructor, a module namespace or a serializer. Such an entry matches neither print and fails untilbun test -u: 96 of 168 probed entries (12 kinds, 8 classes each). The 72 entries with no such neighbour pass. No released Bun writes a mixed entry. The release that first has bun test: Vitest compatibility #44833 is the best one for this PR, bun:test: store the href of a URL in a snapshot instead ofURL {}#48020 and bun:test: store the name and message of a DOMException in a snapshot instead ofDOMException {}#48033 together. If that release ships without them, the mixed entries needbun test -u, or one more replay attempt inExpect::was_written_by_another. bun:test: store the href of a URL in a snapshot instead ofURL {}#48020 describes that attempt and its cost.console.logformatter #46729. Print Bun's snapshot format with theconsole.logformatter #46729 moves Bun's format intosrc/jsc/ConsoleObject.rsand deletesFormatter::print_as. After it, the branch is 4 lines in the hookJestPrettyFormat::print_in_bun_snapshot, beside the branch for DOM nodes that already setssame_as_older_bun. The port is the second commit of branchrobobun/c1db105e/temporal-on-46729, on 070a72c. There the 7 new tests ofsnapshot.test.tsand the 126 tests ofdeep-equals-temporal.test.tspass with no expected text changed. With--timeout 120000,snapshot.test.tshas 196 passes, 1 skip and the one colour failure.FormDatasubclass whosetoJSON()returns a Temporal value stores the value with this PR. After Print Bun's snapshot format with theconsole.logformatter #46729 it storesPlainDate {}:print_privatehands that result toprint_aspast the hook (ConsoleObject.rs:4841). No test pins that text.URL {}#48020 and bun:test: store the name and message of a DOMException in a snapshot instead ofDOMException {}#48033. Git merges this branch with each of the two heads with no conflict. I did not build those merges. If the three ship in two releases, an entry with aURLand a Temporal value that the release between them wrote fails untilbun test -u.Report and reach
Bun.TOML.parsereturns Temporal values since Bun 1.4.0, so a snapshot of a parsed document holds them.bun test ./a.test.jsstores the entry, thenDAY=02 CI=true bun test ./a.test.jsexits 0 on main and on bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105, and 1 on this branch.What a stored
PlainDate {}entry doesCI=trueand withCI=false. The.snapkeeps its sha256.bun test -urewrites the 12 entries once. A second-uchanges 0 bytes. After that a changed value fails 12 of 12.Cost (release builds: main b1305dd, #48105, this PR)
size: 140,296,334 bytes of text, the same as bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105. That is +6,400 over main.toMatchInlineSnapshotcall:BUN_JSC_useJIT=0, concurrent GC off,perf_event_open, the difference between loops of 100,000 and 200,000 calls, median of 5.{ a: "x", b: [1, 2], c: { d: "y" } }new Intl.Collator()PlainDateDurationZonedDateTimePlainDate, stored asPlainDate {}{ id: 1, when: PlainDate }PlainDate {}toEqualare those of bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105 within that spread.Tag::Objectinstantiation ofprint_asstays 152 bytes. The deepest nested object that prints before theRangeError: 5,299 to 5,304 levels, against 5,302 to 5,303 on main.SnapshotFormat__temporalTextfor 100 prints (lldb hit counts, debug build): 0 for the object literal, 100 fornew Intl.Collator(), 100 for the sample ofexpect.objectContaining()in a failingtoEqual.PlainDatevalues (lldb hit counts, debug builds, main against this PR): property walks 100 to 0, class-name lookups 100 to 0, calls of the new function 100.malloccalls for each printed value (ASAN statistics withMalloc=1, 4,000 prints, main against this PR):PlainDate3.2 to 4.2,Duration3.2 to 3.0,ZonedDateTime3.5 to 7.2, aPlainDatewith a calendar 3.0 to 7.3. The same count for a plain number differs by 0.2 between the two builds.did_older_bun_printcalls (lldb hit counts): 0 in a run of 8 entries that match, 8 in a run where each of the 8 values changed, 8 in a run of 8 entries that Bun 1.4.3 stored.bun test -uover the snapshot and printing suites of this repo (10 files, 430 tests): the.snapfiles are byte-identical after a build of main and after this PR.Tests
snapshot.test.ts: 7 new tests in onedescribe. 6 fail on bun:test: print the value of a Temporal object in failure messages instead ofPlainDate {}#48105 and on main. The seventh holds entries that an older Bun wrote, in a test of"vitest", and passes on both.like_older_buncondition, or withoutsame_as_older_bun = false, the 3 tests of older entries fail.bun test -uon the file changes no line of the new tests.toJSONandtoStringof the 8 classes (0 calls), an owntoJSONandtoStringthat throw, the two remedies forTemporal.Now, a class shaped like a polyfill, a serializer, and a.snapwith a Jest header.deep-equals-temporal.test.ts: the test that pinnedPlainDate {}for a snapshot now pins the value.snapshot.test.tson the debug build with--timeout 120000: 186 pass, 1 skip, 1 fail. The one iserror snapshots, which expects colour and fails on main in this container too. WithFORCE_COLOR=1, as in CI, 187 pass and 1 skip. With the default timeout, three runs of main failed 11, 8 and 7 tests and three runs of this branch failed 7, 8 and 7: the colour test and tests that need more than their 5 seconds on this container's debug build.deep-equals-temporal.test.ts(126 pass),expect.test.js(464),mock-fn.test.js(848),dom-nodes.test.ts(79),dom-nodes-jest.test.js(140),diffexample.test.ts(6),bun-snapshots.test.ts,existing-snapshots.test.ts,new-snapshot.test.ts,toml.test.ts(104),toml-test-suite.test.ts(708).Downsides in detail
Temporal.Now: a snapshot that holds such a value passed for any value. After-uit fails on each run. The docs name the two remedies: a property matcher on the property that holds the value,expect({ at: Temporal.Now.instant() }).toMatchSnapshot({ at: expect.any(Temporal.Instant) }), orsetSystemTime().{ year: expect.any(Number) }, is checked and not stored, as for every object that is not plain.printerargument prints in the format of Jest. It stores"2020-01-02"for a Temporal value inside the serialized value, as on main.PlainDate {}. The docs point to a serializer.$$typeofof a React element stores<unknown />, as on main.Tag::getgives the JSX tag to a value of any class that has that property.Self-review
25 concerns, 23 addressed. The two largest: split the failure messages (#48105) from the stored text (this PR), and build the port on #46729. Not taken:
Bun.deepEqualsreads natively. The other classes belong to bun:test: print Headers entries and the href of a URL in failure messages #47611, bun:test: store the href of a URL in a snapshot instead ofURL {}#48020 and bun:test: store the name and message of a DOMException in a snapshot instead ofDOMException {}#48033.PlainDateTime.prototype.toJSON(). It is another repository and another defect.URL {}#48020, which name Format Temporal values in console.log, util.inspect, and test pretty-format #37043 as the owner of the Temporal classes. A comment on Format Temporal values in console.log, util.inspect, and test pretty-format #37043 points here.