console: match Node.js output for timeEnd/timeLog (stdout, "label: duration", 1000 ms threshold) - #34240
console: match Node.js output for timeEnd/timeLog (stdout, "label: duration", 1000 ms threshold)#34240robobun wants to merge 2 commits into
Conversation
console.timeEnd and console.timeLog wrote "[<n>ms] label" to stderr, scaling to seconds only above 1500 ms. Node.js documents and prints "label: <n>ms" on stdout, with seconds kicking in at 1000 ms (and m:ss.mmm / h:mm:ss.mmm beyond that), so shell redirects and log parsers written against the documented shape saw nothing. Route both paths through the console's stdout writer and format with a Node-compatible "label: <duration>" helper that mirrors Node's internal/util/debuglog.js formatTime (ms / s / m:ss.mmm / h:mm:ss.mmm). timeLog's extra arguments now follow the duration and pick stdout's colour setting.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 11:38 AM PT - Jul 15th, 2026
❌ @robobun, your commit c3b6b14 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34240That installs a local version of the PR into your bun-34240 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The top-level | in DURATION was ungrouped, so when interpolated into a larger pattern the surrounding anchors only attached to the first/last alternative. Wrap in (?:...). Also relax the seconds assertions from 1\. to \d+\. so a descheduled busy-wait on a loaded runner cannot spuriously fail; the property under test is the seconds unit, not the leading digit.
There was a problem hiding this comment.
Thanks for addressing both nits in c3b6b14 — the DURATION alternation is now grouped and the seconds-regex flake window is gone. I didn't find anything else in this pass, but leaving the merge to a maintainer since this changes user-visible output (stream + format) and supersedes #32423, which took a narrower approach.
What was reviewed:
write_timer_label_and_durationformatting against Node'sformatTime(ms/s/m:ss/h:mm:ss branches, trailing-zero stripping viafmt::double)timeEnd/timeLogwriter switch:vm_console_mutvs raw-pointer deref pattern is preserved from the pre-PRerror_writer()path; no new aliasing acrossfmt.format(...)- Grepped for other tests/snapshots asserting the old
[Nms] labelshape — none found (test-console-with-frozen-intrinsics.jscalls but doesn't assert output)
Extended reasoning...
Overview
The PR rewires Bun__ConsoleObject__timeEnd and Bun__ConsoleObject__timeLog in src/jsc/ConsoleObject.rs to write label: <duration> to the console's stdout writer instead of [<duration>] label to stderr via Output::print_elapsed. A new write_timer_label_and_duration helper implements Node's formatTime rules (ms → s at 1000 ms; m:ss.mmm / h:mm:ss.mmm above one minute). Tests in console-timeLog.test.ts are updated for the new stream/format and expanded to cover the threshold, default label, and duplicate/unknown-label cases; the .expected.txt snapshot is regenerated.
What changed since my last review
Both prior nits were fixed in c3b6b14: the DURATION regex now wraps its alternation in (?:...) so interpolated anchors bind to every branch, and the two seconds-format assertions use \\d+\\.\\d{3}s instead of pinning the leading 1. Both inline threads are resolved. No new bugs surfaced from the bug-hunting pass on the current head.
Security risks
None. This is output formatting to stdout; no untrusted-input parsing, auth, or filesystem paths are involved. The unsafe blocks (vm_console_mut / raw (*console).writer() deref) are the same pre-existing pattern that was already used for error_writer(), with the same SAFETY comments; the change only swaps which writer is returned and moves acquisition a few lines earlier. timeEnd uses vm_console_mut (exclusive borrow, no re-entry after acquisition) while timeLog keeps the raw-pointer approach so the writer borrow doesn't span fmt.format(...) — structurally identical to before.
Level of scrutiny
Medium. The implementation is small, well-tested (9 cases, verified failing on main and passing under ASAN per the PR gate), and the unsafe pattern is unchanged. However, this is a user-visible behavior change: anyone parsing Bun's old [Nms] label on stderr will break. Issue #12031 requests exactly this and #32423 already attempted the stream switch, so organizational intent is clear — but #32423 deliberately kept Bun's format, and this PR goes further (full Node grammar including the untested m:ss.mmm / h:mm:ss.mmm branches). A maintainer should confirm which approach they want before merge.
Other factors
I checked for collateral test breakage from the format change: test/js/node/test/parallel/test-console-with-frozen-intrinsics.js calls console.time* but asserts nothing about the output, and no other test or snapshot in the repo matches the old [Nms] console-timer shape. The tests follow test/CLAUDE.md conventions (bunExe/bunEnv, -e for single-file, it.concurrent, drain pipes with Promise.all, exit-code asserted last). Given all feedback is addressed and the code looks correct, this is a product-decision sign-off rather than a code-quality block.
|
CI on build 73285 is green for
Diff is ready for review. |
|
Closing as a duplicate of #37128, which reworks console output as a whole and includes everything here: timeEnd/timeLog on stdout, the |
1 participant
console.timeEndandconsole.timeLogwrote[<n>ms] labelto stderr, scaling to seconds only above 1500 ms. Node.js documents and printslabel: <n>mson stdout, scaling at 1000 ms, so shell redirects and log parsers written against the documented shape received nothing from Bun.Supersedes #32423, which switched the stream but kept the Bun-specific format and threshold.
Repro
And with a >1 s timer, Node prints
sc: 1.201swhile Bun printed[1199.23ms] sc(no scaling until 1500 ms).Cause
Bun__ConsoleObject__timeEnd/timeLoginsrc/jsc/ConsoleObject.rscalledOutput::print_elapsed, which is the CLI-style stderr helper insrc/bun_core/output.rs:pretty_error!with a[{:.2}ms]template and a0..=1500match arm for the ms/s split. The label was appended afterwards viaprint_error{,ln}, andtimeLog's extra arguments went through the console'serror_writer().Fix
Add
write_timer_label_and_duration, a dedicated stdout formatter that mirrors Node'sinternal/util/debuglog.jsformatTime:< 1000 ms:Number(ms.toFixed(3)) + "ms"(round to three decimals, trailing zeros stripped)< 60 s:seconds.toFixed(3) + "s"< 1 h:m:ss.mmm (m:ss.mmm)>= 1 h:h:mm:ss.mmm (h:mm:ss.mmm)timeEndandtimeLognow writelabel: <duration>to the console's stdout writer;timeLog's extra arguments follow the duration on the same line and pick the stdout colour setting.Unchanged behaviours, now covered by tests:
console.time(label)keeps the original timer.timeEnd/timeLogon an unknown label produce no stdout output.Verification
test/js/web/console/console-timeLog.test.tsasserts the stream, thelabel: <duration>grammar, the 1000 ms threshold, the default label, and the duplicate/unknown-label cases.USE_SYSTEM_BUN=1 bun test test/js/web/console/console-timeLog.test.tsfails 8/9bun bd test test/js/web/console/console-timeLog.test.tspasses 9/9bun bd test test/js/web/console/passes 16/16Fixes #12031
[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file