fix(console): prepend "Assertion failed: " prefix to assert messages with data - #36860
fix(console): prepend "Assertion failed: " prefix to assert messages with data#36860aalhadxx wants to merge 2 commits into
Conversation
Walkthrough
ChangesConsole assertion output
Possibly related PRs
Suggested reviewers: 🚥 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 |
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 `@src/jsc/ConsoleObject.rs`:
- Line 541: Update the prefix-writing logic in the surrounding assertion-output
function so the result of writer.write_all(text.as_bytes()) is handled
explicitly. Propagate failures through the function’s existing typed error path,
preserving the behavior that the assertion body is not emitted when writing the
required prefix fails.
🪄 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: 97b35baa-dfb8-4e88-9e59-5bff6f34f3d3
📒 Files selected for processing (1)
src/jsc/ConsoleObject.rs
| } else { | ||
| "Assertion failed: " | ||
| }; | ||
| let _ = writer.write_all(text.as_bytes()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate the prefix write error.
Line 541 discards the writer.write_all result. If the stream fails, the function continues and can emit the assertion body without the required prefix. Propagate the error through the existing typed error path, or explicitly handle only an allowed stream error.
As per coding guidelines, I/O failures must be propagated explicitly instead of being discarded.
🤖 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 `@src/jsc/ConsoleObject.rs` at line 541, Update the prefix-writing logic in the
surrounding assertion-output function so the result of
writer.write_all(text.as_bytes()) is handled explicitly. Propagate failures
through the function’s existing typed error path, preserving the behavior that
the assertion body is not emitted when writing the required prefix fails.
Source: Coding guidelines
…with data console.assert() with additional arguments was not prepending the "Assertion failed: " prefix before the formatted arguments, unlike Node.js and browser behavior. Fixes oven-sh#19953 Signed-off-by: Aalhad <aalhadxx@users.noreply.github.com>
2edb470 to
619a0a7
Compare
|
Addressed the review comment: the result of writer.write_all is now propagated through the existing error path (writer.write_all(...)?). The prefix is only written when there are assertion data to print, matching Node/browser behavior. |
775c750 to
350f5d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jsc/ConsoleObject.rs (1)
541-541: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate the prefix write error.
Line 541 discards the
writer.write_allresult. If the stream fails, the function continues and can return success after emitting incomplete assertion output. Propagate the error through the existingJsResultpath before callingformat2.This is the same unresolved issue reported in the previous review. As per coding guidelines, I/O failures must be propagated explicitly instead of being discarded.
🤖 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 `@src/jsc/ConsoleObject.rs` at line 541, Update the output-writing flow around writer.write_all so its I/O error is propagated through the existing JsResult path before format2 is called; remove the discarded-result handling and preserve normal formatting only when the prefix write succeeds.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.
Duplicate comments:
In `@src/jsc/ConsoleObject.rs`:
- Line 541: Update the output-writing flow around writer.write_all so its I/O
error is propagated through the existing JsResult path before format2 is called;
remove the discarded-result handling and preserve normal formatting only when
the prefix write succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9174e0f-5aad-4ffe-b57c-73ec7e7dddde
📒 Files selected for processing (1)
src/jsc/ConsoleObject.rs
|
@coderabbitai Good catch. The latest commit e951d30 propagates the writer.write_all(prefix) result through the existing error path (writer.write_all(prefix)?;), and only writes the prefix when there is assertion data. Let me know if you'd like any further changes. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Thanks for the PR. This is being fixed as part of #37128, which reworks console output as a whole. It adds the |
Closes #19953
What does this PR do?
console.assert()with additional data arguments now prepends"Assertion failed: "before the formatted arguments, matching Node.js and browser behavior.How did you verify your code works?
es-module-lexer+magic-stringsnippet and confirmed non-empty assertions receive the new prefix while empty assertions keep the existing standalone failure message.Related PRs
src/jsc/ConsoleObject.rsforconsole.asserthandling.