fix(console): send console.trace to stderr with a "Trace:" prefix - #32638
fix(console): send console.trace to stderr with a "Trace:" prefix#326380xfandom wants to merge 4 commits into
Conversation
console.trace wrote the formatted arguments and stack trace to stdout with no prefix. Node writes them to stderr, prefixed with "Trace: " (or a bare "Trace" when called with no arguments). Route the Trace message type to the stderr writer and emit the prefix before the formatted arguments. Fixes oven-sh#19952
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
Changesconsole.trace stderr routing and prefix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/console/console.test.ts`:
- Around line 115-126: The test cases "no arguments prints bare 'Trace'" and
"applies format specifiers" do not verify the subprocess exit code, which means
they could pass even if the execution fails. In both tests, destructure exitCode
from the run() result (along with stdout and stderr), and add an
expect(exitCode).toBe(0) assertion after the output assertions to ensure the
subprocess executed successfully before verifying the console output.
🪄 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: 88c702e5-5d6d-427c-8592-7a792c2ff0db
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/node/console/console.test.ts
|
Addressed in the latest commit: the no-args and format-specifier cases now assert |
|
I ended up writing the same fix before finding this PR, so I'm not opening a competing one. Branch is at One behavior worth folding in either way: Node puts the label after the console.group("G");
console.trace("x");
console.trace();
console.groupEnd();It happens because Happy either way, just flagging so it doesn't get lost. |
format2 writes the indent as its first bytes, so writing "Trace: " before calling it put the label ahead of the group indent, and the no-args path skipped the indent entirely. Thread the label through FormatOptions as a prefix that format2 emits right after the indent, and indent the bare header, matching Node.
|
Good catch — folded in as d699e1b. Threaded the label through Added a test covering the grouped case. Full console suites still pass (90 pass / 0 fail). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/node/console/console.test.ts (2)
107-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun independent subprocess cases concurrently.
These tests use isolated child processes and can use
test.concurrentwithout sharing state. This avoids unnecessary serialization.As per coding guidelines, independent subprocess suites should use
test.concurrent.Proposed fix
- test("goes to stderr, not stdout", async () => { + test.concurrent("goes to stderr, not stdout", async () => { - test("no arguments prints bare 'Trace'", async () => { + test.concurrent("no arguments prints bare 'Trace'", async () => { - test("applies format specifiers", async () => { + test.concurrent("applies format specifiers", async () => { - test("label goes after the console.group indent", async () => { + test.concurrent("label goes after the console.group indent", async () => {🤖 Prompt for 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. In `@test/js/node/console/console.test.ts` around lines 107 - 141, Update the four independent console.trace tests in the surrounding test suite to use test.concurrent, preserving each test’s existing assertions and subprocess setup. Do not alter the test behavior or shared state handling.Source: Coding guidelines
97-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpread
bunEnvfor the child environment.Use
env: { ...bunEnv }instead of passing the shared harness object directly. This preserves test isolation if per-test environment overrides are added, especially once these cases run concurrently.As per coding guidelines, “Follow existing harness conventions: spread
bunEnv.”Proposed fix
- env: bunEnv, + env: { ...bunEnv },🤖 Prompt for 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. In `@test/js/node/console/console.test.ts` around lines 97 - 100, Update the Bun.spawn invocation in the console test to pass a shallow copy of bunEnv via object spread in the env option, preserving the existing child process configuration and isolating per-test environment changes.Source: Coding guidelines
🤖 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 `@test/js/node/console/console.test.ts`:
- Around line 130-139: Update the test case around console.group and
console.trace to validate the ordered grouped trace headers rather than merely
checking their presence. Extract header lines from stderr while ignoring
stack-frame text, then assert the exact sequence “ Trace: x”, “ Trace”, and
“Trace: top”; keep the existing stdout and exitCode assertions unchanged.
---
Outside diff comments:
In `@test/js/node/console/console.test.ts`:
- Around line 107-141: Update the four independent console.trace tests in the
surrounding test suite to use test.concurrent, preserving each test’s existing
assertions and subprocess setup. Do not alter the test behavior or shared state
handling.
- Around line 97-100: Update the Bun.spawn invocation in the console test to
pass a shallow copy of bunEnv via object spread in the env option, preserving
the existing child process configuration and isolating per-test environment
changes.
🪄 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 Plus
Run ID: e13aa570-4ab4-42cc-a12c-135a0332c7d9
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/node/console/console.test.ts
|
Addressed in c02d6da — the group-indent test now extracts the header lines and asserts the exact ordered sequence |
|
Triage note: the stderr half of this is also being fixed in #37128, which reworks console output as a whole. What #37128 does not add is the Once #37128 lands this will need a rebase: |
2 participants
What does this PR do?
Fixes #19952.
console.trace()wrote its output to stdout with no prefix; Node writes to stderr prefixed withTrace:.The native console handler (
src/jsc/ConsoleObject.rs) only routedWarning/Error/Assertmessages to the stderr writer, and never emitted a label for theTracemessage type. This change routesTraceto the stderr writer (stream selection + color mode) and printsTrace:before the formatted arguments, or a bareTracewhen called with no arguments — matching Node:The
.zigreference sibling has the same gap; left untouched since it is not compiled.How did you verify your code works?
Added
console.tracetests intest/js/node/console/console.test.tsthat spawn a subprocess and assert stdout is empty, stderr starts withTrace: hi\n/ bareTrace\n(no-args) and contains the stack frames, plus format-specifier handling. Tests pass on the debug build and fail on the releasedbun(USE_SYSTEM_BUN=1). Existing console suites (console.test.ts,console-log.test.ts) still pass: 14 / 14.