-
Notifications
You must be signed in to change notification settings - Fork 5k
fix(console): send console.trace to stderr like Node #39240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,16 @@ | ||||||||||||||||||
| import { expect, test } from "bun:test"; | ||||||||||||||||||
| import { bunEnv, bunExe } from "harness"; | ||||||||||||||||||
|
|
||||||||||||||||||
| test("console.trace writes to stderr, not stdout", async () => { | ||||||||||||||||||
| await using proc = Bun.spawn({ | ||||||||||||||||||
| cmd: [bunExe(), "-e", `console.trace("hello")`], | ||||||||||||||||||
| env: bunEnv, | ||||||||||||||||||
| stdout: "pipe", | ||||||||||||||||||
| stderr: "pipe", | ||||||||||||||||||
| }); | ||||||||||||||||||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||||||||||||||||||
| expect(exitCode).toBe(0); | ||||||||||||||||||
| expect(stdout).toBe(""); | ||||||||||||||||||
| expect(stderr).toContain("hello"); | ||||||||||||||||||
| expect(stderr.toLowerCase()).toMatch(/trace/i); | ||||||||||||||||||
|
Comment on lines
+12
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert stream output before The test checks Proposed fix- expect(exitCode).toBe(0);
expect(stdout).toBe("");
expect(stderr).toContain("hello");
expect(stderr.toLowerCase()).toMatch(/trace/i);
+ expect(exitCode).toBe(0);As per coding guidelines, subprocess tests must drain stdout, stderr, and process exit concurrently and assert the combined result and ordered stage outputs. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||
| }); | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve
MessageType::Assertwhen addingMessageType::Trace.The
use_stderrpredicate still includesMessageType::Asserton Line 411-412. These two predicates omit it. An argument-bearing assertion with a normal level can therefore select the stdout color policy andwriter(), while the stream lock targets stderr.Keep the existing assertion case and add the trace case:
Proposed fix
Also applies to: 456-457
🤖 Prompt for AI Agents