Skip to content
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

Pretty print assertion failures in tests #79001

Closed
wants to merge 11 commits into from

Conversation

de-vri-es
Copy link
Contributor

@de-vri-es de-vri-es commented Nov 12, 2020

This PR aims to allow the test framework to pretty-print assertion failures with colors and a more readable format than the current panic messages. It is meant to work towards #44838 (without actually modifying assert!() itself at this point).

The PR adds a new extra_info field to PanicInfo. Currently, extra_info is an enum that can only contain &AssertInfo, but it is meant to be extendable with different variants in the future. It could potentially be used to record details for other sources of panics too. The panic handler in std ensures that the extra_info is also passed to the panic hook unmodified.

I also looked into passing the AssertInfo as panic payload rather than a new field on PanicInfo. However, casting an &AssertInfo to &dyn Any would require AssertInfo: 'static, but it borrows the evaluated expressions.

The PR adds an new unstable --pretty-print-assertions flag to test binaries to enable the pretty printing. When used, the test runner installs a panic hook that does the pretty-printing. The printing is done with print!()/println!() to ensure the output is still captured by the test framework. That means that the color codes need to be included in the print!() calls, which is only possible with terminals that support ANSI color codes. Support for ANSI color codes is checked before enabling them.

For now, the PR only modifies assert_eq!() and assert_ne!(), since they're much easier. Next up would be assert!() parsing the input expression and generating the proper AssertInfo for the panic.

And now some pictures!

Without pretty-printing:

not-pretty

pretty

The pretty printing is hidden behind an unstable command line flag to allow more time to tweak the format, and of course to actually implement expression parsing in assert!().

I imagine there is a lot of bike-shedding potential on the exact output format. Initially I wanted to go with the format used by assert2, but I figured that might be too opinionated.

r? @m-ou-se

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2020
@m-ou-se m-ou-se added A-libtest Area: `#[test]` / the `test` library T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 12, 2020
@de-vri-es de-vri-es force-pushed the pretty-print-assertions branch 3 times, most recently from e9024e7 to 22c575a Compare November 12, 2020 21:27
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Here are some first review comments:

library/core/src/panic.rs Outdated Show resolved Hide resolved
library/core/src/panic/assert_info.rs Outdated Show resolved Hide resolved
library/core/src/panic/assert_info.rs Outdated Show resolved Hide resolved
library/core/src/panicking.rs Outdated Show resolved Hide resolved
library/term/src/win.rs Outdated Show resolved Hide resolved
@@ -31,4 +31,5 @@ fn main() {
//~^ ERROR binary operation `==` cannot be applied to type `fn(usize) -> Foo {Foo::Bar}` [E0369]
//~| ERROR `fn(usize) -> Foo {Foo::Bar}` doesn't implement `Debug` [E0277]
//~| ERROR `fn(usize) -> Foo {Foo::Bar}` doesn't implement `Debug` [E0277]
//~| ERROR `fn(usize) -> Foo {Foo::Bar}` doesn't implement `Debug` [E0277]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now duplciates an error. Can that be avoided?

Copy link
Contributor Author

@de-vri-es de-vri-es Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The expression is still used to create the same panic message as before, but now also used to form a &dyn Debug in the AssertInfo.

I'd be happy to avoid this if possible though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe it can be done by using the same &dyn Debug also for the panic message. Not sure if that is desirable though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the macro to assign left_val and right_val to a &dyn Debug once, and then use that for the rest of the code. The extra error is gone that way.

Interestingly, the compiler now only gives an error once per type. So if left and right are different types (and both don't implement Debug), it gives an error for both. If they are the same type, only one error is generated. This seems like nice behaviour, so I added a UI test for it.

library/core/src/macros/mod.rs Outdated Show resolved Hide resolved

fn print_pretty_binary_assertion(assert: &BinaryAssertion<'_>) {
eprintln!(
"{bold}Assertion:{reset}\n {cyan}{left} {bold}{blue}{op}{reset} {yellow}{right}{reset}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the precedence of the comparison operator and any operators in the expressions on either side?

assert_eq!(true && false, false || true) might give surprising results (especially if colors are not enabled).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. Putting parenthesis around the expressions is possible, or it can be split further into separate lines for left and right.

I'll probably try out both to see how they look.

// The reborrows below are intentional. Without them, the stack slot for the
// borrow is initialized even before the values are compared, leading to a
// noticeable slow down.
$crate::panicking::start_panic(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a new panicking function here means clippy will no longer recognize this panic. Once rust-lang/rust-clippy#6310 lands, this should be easy to fix by adding start_panic to the list of panic functions it recognizes.

Copy link
Member

@jyn514 jyn514 Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed in #79145, so you just need to update the functions clippy recognizes.

@de-vri-es de-vri-es force-pushed the pretty-print-assertions branch 9 times, most recently from c590560 to 97d31d1 Compare November 15, 2020 20:41
library/test/src/lib.rs Outdated Show resolved Hide resolved
src/tools/compiletest/src/main.rs Outdated Show resolved Hide resolved
@a1phyr
Copy link
Contributor

a1phyr commented Nov 18, 2020

I tried to enhance assert_eq! before (clearly not as good as you did, though), and it had a very bad impact on build times (see #73968).

I think we should do a perf run on this.

@de-vri-es
Copy link
Contributor Author

de-vri-es commented Nov 18, 2020

I tried to enhance assert_eq! before (clearly not as good as you did, though), and it had a very bad impact on build times (see #73968).

I think we should do a perf run on this.

Eep, if that was already bad I'm a little worried what this will do. I'm not sure how to do a perf run, but that sounds like a good idea.

In the end I do think that increasing compile times is unavoidable for RFC 2011. Maybe it would be a solution to hide the new macro implementation behind #[cfg(test)], or possibly something more granular.

@jyn514
Copy link
Member

jyn514 commented Nov 18, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 18, 2020

⌛ Trying commit f56df5fdb6dc062411b4254f26989e7a1555b145 with merge c7bcb4ce327047165c733e403fbe3d345c7376a0...

@bors
Copy link
Contributor

bors commented Nov 18, 2020

☀️ Try build successful - checks-actions
Build commit: c7bcb4ce327047165c733e403fbe3d345c7376a0 (c7bcb4ce327047165c733e403fbe3d345c7376a0)

@rust-timer
Copy link
Collaborator

Queued c7bcb4ce327047165c733e403fbe3d345c7376a0 with parent 8d2d001, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c7bcb4ce327047165c733e403fbe3d345c7376a0): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@m-ou-se m-ou-se removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2020
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2021
@JohnCSimon
Copy link
Member

Ping again from triage:
@de-vri-es Unfortunately there's another merge conflict. Can you please rebase the branch again?

@jakoschiko
Copy link
Contributor

I'll try to resolve the conflicts.

@bors
Copy link
Contributor

bors commented Oct 20, 2021

☔ The latest upstream changes (presumably #90067) made this pull request unmergeable. Please resolve the merge conflicts.

de-vri-es and others added 11 commits October 20, 2021 21:22
The assertion details are added as a new enum field, allowing for future
extensions with different information for other sources of panics.
For example, `.unwrap()` could extend the enum to store a `&dyn Error`
in the panic info.

The panic handler in `std` passes the additional info unmodified to the
panic hook.
Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
And use a comma to separate arguments of `assert_eq` and `assert_ne`.
This has two advantages:
 * The printed text more closely matches the actual source code.
 * It avoids changing the meaning of $left == $right if $left or $right
   contain operators themselves.
`GetConsoleMode` was declared twice with different signatures.
The signatures have contained different definitions of `HANDLE`.
@bors
Copy link
Contributor

bors commented Oct 27, 2021

☔ The latest upstream changes (presumably #90273) made this pull request unmergeable. Please resolve the merge conflicts.

@jakoschiko
Copy link
Contributor

It seems that the current conflict cannot be resolved by simple rebasing. The problem is the ongoing constification of panic_fmt.

master:

#[rustc_do_not_const_check] // hooked by const-eval
pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {

feature branch:

pub fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
    panic_description(PanicDescription::Message(&fmt))
}

fn panic_description(description: PanicDescription<'_>) -> ! {

I think it would be necessary to make both functions const and change the const-eval hook. But it would be better to wait until the other work on panic_fmt has been completed.

@de-vri-es @m-ou-se
Maybe the time has come to close this PR.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@de-vri-es

jakoschiko commented on Nov 14, 2021
Maybe the time has come to close this PR.

Seems like this PR has stalled. What would you like to do?

@Dylan-DPC Dylan-DPC added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 19, 2022
@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2022
@Dylan-DPC
Copy link
Member

Closing this pr due to inactivity. Thanks for taking the time to contribute.

@Dylan-DPC Dylan-DPC closed this Mar 3, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.