-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
do not print panic message on doctest failures #60549
Conversation
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.
Looks like this fixes #48394, which i'd originally closed as "not a bug". >_>
I'll take another look at this soon, but i like what i see so far! I'm going to rope in the rest of @rust-lang/rustdoc and @rep-nop since this was a big doctest thing for this year. It doesn't even seem like that much of a code change, either!
} | ||
} | ||
|
||
panic::resume_unwind(box ()); |
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.
This is clever! I wouldn't think that resume_unwind
would suppress the panic message, but i guess that that only happens in the panic hook, not in the pre-main
landing pad.
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.
Yeah! I got the idea from this PR: #59990
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.
Would have been nice to also copy the explanatory comment from that PR. :)
That's way better! Thanks a lot! |
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.
One little nit, otherwise the rest of this looks great!
if !stdout.is_empty() || !stderr.is_empty() { | ||
if !stdout.is_empty() { | ||
eprintln!("{}", stdout); | ||
} |
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.
I feel like this printout would be better served by bringing back the "Test executable failed" message and maybe also prefixing each printout with stdout:\n{}
and stderr:\n{}
so it's obvious what's going on.
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.
How about including the exit code as well?
src/librustdoc/test.rs
Outdated
|
||
if let Err(err) = res { | ||
match err { | ||
TestFailure::CompileError => (), // The compiler errors are enough |
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.
I think there should still be a message like "Couldn't compile the test." in this case.
f1c60e7
to
0a1ee14
Compare
Comments addressed. |
☔ The latest upstream changes (presumably #61027) made this pull request unmergeable. Please resolve the merge conflicts. |
0a1ee14
to
89d437e
Compare
@QuietMisdreavus Rebased. |
Thanks! @bors: r+ |
📌 Commit 89d437e has been approved by |
…illaumeGomez do not print panic message on doctest failures This PR cleans up rustdoc test output by silently unwinding on failure instead of using `panic!`. It also improves the clarity and consistency of the output on test failure, and adds test cases for failure modes that were previously untested.
…illaumeGomez do not print panic message on doctest failures This PR cleans up rustdoc test output by silently unwinding on failure instead of using `panic!`. It also improves the clarity and consistency of the output on test failure, and adds test cases for failure modes that were previously untested.
…illaumeGomez do not print panic message on doctest failures This PR cleans up rustdoc test output by silently unwinding on failure instead of using `panic!`. It also improves the clarity and consistency of the output on test failure, and adds test cases for failure modes that were previously untested.
Rollup of 7 pull requests Successful merges: - #60549 (do not print panic message on doctest failures) - #60885 (strip synstructure consts from compiler docs) - #61217 (Account for short-hand init structs when suggesting conversion) - #61261 (is_union returns ty to avoid computing it twice) - #61293 (Print const generics properly in rustdoc) - #61310 (split libcore::mem into multiple files) - #61313 (Simplify Set1::insert) Failed merges: r? @ghost
This PR cleans up rustdoc test output by silently unwinding on failure instead of using
panic!
. It also improves the clarity and consistency of the output on test failure, and adds test cases for failure modes that were previously untested.