cli: return non-zero for uncaught runtime exceptions#4964
Open
SergioChan wants to merge 7 commits intoboa-dev:mainfrom
Open
cli: return non-zero for uncaught runtime exceptions#4964SergioChan wants to merge 7 commits intoboa-dev:mainfrom
SergioChan wants to merge 7 commits intoboa-dev:mainfrom
Conversation
Test262 conformance changes
Fixed tests (2):Broken tests (175):Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4964 +/- ##
===========================================
+ Coverage 47.24% 58.74% +11.50%
===========================================
Files 476 558 +82
Lines 46892 61380 +14488
===========================================
+ Hits 22154 36059 +13905
- Misses 24738 25321 +583 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
I don't think this is fully working ➜ boa git:(pr-4964) echo 'throw Error("nooo")' | cargo run
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.13s
Running `target/debug/boa`
Uncaught Error: Error: nooo (unknown at :1:12)
at <main> (unknown at :1:12)
➜ boa git:(pr-4964) echo $?
0
➜ boa git:(pr-4964) |
jedel1043
added
A-Bug
Author
|
Thanks for the repro — you were right. The previous patch only returned a non-zero status for file execution errors. I pushed a follow-up commit that propagates those errors out of
What changed:
Validation note:
|
Member
|
Yeah a test would be ideal, using something like |
jedel1043
requested changes
Mar 11, 2026
Author
|
Great call — I added integration regression coverage using What I changed
Validation
|
Author
|
Pushed follow-up commit What changed
This keeps the |
Member
|
You also need to commit your changes to the |
Author
|
Thanks for the review. I鈥檓 working through the requested updates and will post a focused patch shortly. |
jedel1043
reviewed
Mar 13, 2026
cli/src/main.rs
Outdated
| Err(ref v) => printer.print(uncaught_error(v)), | ||
| Err(v) => { | ||
| printer.print(uncaught_error(&v)); | ||
| return Err(v.into_erased(context).into()); |
Member
There was a problem hiding this comment.
Tested the code and directly returning the error makes it show up two times in stderr. We might have to adjust the returned error such that it doesn't reference the already printed error.
Author
|
Thanks for the pointer on I pushed commit
This keeps the same regression coverage (stdin and Validation run in this update:
I couldn’t run |
Member
|
You didn't address my comment though |
Author
|
You’re absolutely right — I missed that thread. Thanks for the nudge. I’ll address your remaining comment directly and push a follow-up patch shortly. |
github-actions
bot
added
C-Tests
Author
|
Thanks for the follow-up — I tracked down the missed review point about duplicated stderr output. I just pushed What changed
This preserves the non-zero exit behavior while avoiding duplicated detailed error text in stderr. Validation
|
Author
|
Quick follow-up on the outstanding changes-requested thread: commit 62e25c9 keeps the uncaught-error printing path but returns sentinel errors to avoid duplicated stderr output, while preserving non-zero exits.\n\n@jedel1043 could you re-check this when you have time and confirm whether this resolves your remaining concern? |
Author
|
Quick follow-up on the CHANGES_REQUESTED items: I switched the -e/stdin paths to return a short eyre!(...) after printing the uncaught error so we avoid bubbling the full engine error twice on stderr, while keeping evaluate_file propagation as-is for file execution semantics.\n\nCould you re-check if this matches what you had in mind, or should I also switch the file path to a shortened propagated error? |
Author
|
Pushed follow-up commit What changed
WhyThis keeps the non-zero exit behavior from #4962 while avoiding duplicate detailed error output on stderr in the file path (matching the stdin / Validation
|
SergioChan
force-pushed
the
fix/issue-4962-exitcode
branch
from
17c35d5 to
fa38eac
Compare
Author
|
Quick update: I rebased this branch onto the latest What changed
Validation
|
SergioChan
force-pushed
the
fix/issue-4962-exitcode
branch
from
fa38eac to
be429ba
Compare
Author
|
Rebased this branch onto the latest No intentional functional changes were made in this update; this was a branch-refresh push to keep the PR current for review. Validation in this run:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes/closes #4962.
It changes the following:
evaluate_filewhen script execution raises an uncaught runtime exception.Validation
cargo) is unavailable.Ok(()).