Pretty print assertion failures in tests #79001

de-vri-es · 2020-11-12T20:55:03Z

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:

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

m-ou-se

Thanks for working on this!

Here are some first review comments:

library/core/src/panic.rs

library/core/src/panic/assert_info.rs

library/core/src/panicking.rs

library/term/src/win.rs

m-ou-se · 2020-11-12T21:40:24Z

src/test/ui/issues/issue-59488.rs

@@ -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]


This now duplciates an error. Can that be avoided?

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.

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

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

m-ou-se · 2020-11-12T21:57:33Z

library/test/src/pretty_print_assertion.rs

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


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).

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.

m-ou-se · 2020-11-12T22:56:10Z

library/core/src/macros/mod.rs

+                    // 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(


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.

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

library/test/src/lib.rs

src/tools/compiletest/src/main.rs

a1phyr · 2020-11-18T19:03:03Z

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 · 2020-11-18T21:46:34Z

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 · 2020-11-18T21:49:26Z

@bors try @rust-timer queue

rust-timer · 2020-11-18T21:49:27Z

Awaiting bors try build completion

bors · 2020-11-18T21:49:37Z

⌛ Trying commit f56df5fdb6dc062411b4254f26989e7a1555b145 with merge c7bcb4ce327047165c733e403fbe3d345c7376a0...

bors · 2020-11-18T22:39:05Z

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

rust-timer · 2020-11-18T22:39:07Z

Queued c7bcb4ce327047165c733e403fbe3d345c7376a0 with parent 8d2d001, future comparison URL.

rust-timer · 2020-11-19T01:55:25Z

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

JohnCSimon · 2021-10-03T07:18:46Z

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

jakoschiko · 2021-10-10T20:16:39Z

I'll try to resolve the conflicts.

bors · 2021-10-20T01:43:24Z

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

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 <[email protected]>

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 · 2021-10-27T19:04:23Z

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

jakoschiko · 2021-11-14T11:54:58Z

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 · 2022-02-13T00:44:49Z

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 · 2022-03-03T20:30:51Z

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

rust-highfive assigned m-ou-se Nov 12, 2020

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 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 force-pushed the pretty-print-assertions branch 3 times, most recently from e9024e7 to 22c575a Compare November 12, 2020 21:27

m-ou-se reviewed Nov 12, 2020

View reviewed changes

de-vri-es force-pushed the pretty-print-assertions branch 9 times, most recently from c590560 to 97d31d1 Compare November 15, 2020 20:41

jyn514 reviewed Nov 18, 2020

View reviewed changes

library/test/src/lib.rs Outdated Show resolved Hide resolved

src/tools/compiletest/src/main.rs Outdated Show resolved Hide resolved

m-ou-se mentioned this pull request Nov 18, 2020

Improve assert_eq! and assert_ne! #79100

Merged

m-ou-se removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2020

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

jakoschiko force-pushed the pretty-print-assertions branch from 7fcdc68 to ee2ae4a Compare October 10, 2021 20:52

de-vri-es and others added 11 commits October 20, 2021 21:22

Add function to test if terminal supports ANSI color codes.

0994b8a

Pretty-print assertion failures in test with unstable flag.

d57b2da

Mark panic::ExtraInfo as non-exhaustive.

9596fce

Co-authored-by: Mara Bos <[email protected]>

Add more documentation to AssertInfo and related types.

7f8140e

Replace get_stdout_handle() with io::stdout().as_raw_handle().

5886466

Do not show fake operator in pretty-printed assert_matches failure.

3b03d3a

Try to improve performance by reducing the size of involved types

4b87907

Unify signatures of GetConsoleMode by changing HANDLE to c_void

ec39e42

`GetConsoleMode` was declared twice with different signatures. The signatures have contained different definitions of `HANDLE`.

Bless test output

e0279b3

jakoschiko force-pushed the pretty-print-assertions branch from ee2ae4a to e0279b3 Compare October 20, 2021 20:40

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

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 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2022

Dylan-DPC closed this Mar 3, 2022

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

Kobzol mentioned this pull request Jul 13, 2024

using correct tool mode for run-make-support crate #127677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pretty print assertion failures in tests #79001

Pretty print assertion failures in tests #79001

de-vri-es commented Nov 12, 2020 •

edited

Loading

m-ou-se left a comment

m-ou-se Nov 12, 2020

de-vri-es Nov 12, 2020 •

edited

Loading

de-vri-es Nov 12, 2020

de-vri-es Nov 15, 2020

m-ou-se Nov 12, 2020

de-vri-es Nov 12, 2020

m-ou-se Nov 12, 2020

jyn514 Jan 11, 2021 •

edited

Loading

a1phyr commented Nov 18, 2020

de-vri-es commented Nov 18, 2020 •

edited

Loading

jyn514 commented Nov 18, 2020

rust-timer commented Nov 18, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

rust-timer commented Nov 18, 2020

rust-timer commented Nov 19, 2020

JohnCSimon commented Oct 3, 2021

jakoschiko commented Oct 10, 2021

bors commented Oct 20, 2021

bors commented Oct 27, 2021

jakoschiko commented Nov 14, 2021

JohnCSimon commented Feb 13, 2022

Dylan-DPC commented Mar 3, 2022

Pretty print assertion failures in tests #79001

Pretty print assertion failures in tests #79001

Conversation

de-vri-es commented Nov 12, 2020 • edited Loading

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Nov 12, 2020

Choose a reason for hiding this comment

de-vri-es Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

de-vri-es Nov 12, 2020

Choose a reason for hiding this comment

de-vri-es Nov 15, 2020

Choose a reason for hiding this comment

m-ou-se Nov 12, 2020

Choose a reason for hiding this comment

de-vri-es Nov 12, 2020

Choose a reason for hiding this comment

m-ou-se Nov 12, 2020

Choose a reason for hiding this comment

jyn514 Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

a1phyr commented Nov 18, 2020

de-vri-es commented Nov 18, 2020 • edited Loading

jyn514 commented Nov 18, 2020

rust-timer commented Nov 18, 2020

bors commented Nov 18, 2020

bors commented Nov 18, 2020

rust-timer commented Nov 18, 2020

rust-timer commented Nov 19, 2020

JohnCSimon commented Oct 3, 2021

jakoschiko commented Oct 10, 2021

bors commented Oct 20, 2021

bors commented Oct 27, 2021

jakoschiko commented Nov 14, 2021

JohnCSimon commented Feb 13, 2022

Dylan-DPC commented Mar 3, 2022

de-vri-es commented Nov 12, 2020 •

edited

Loading

de-vri-es Nov 12, 2020 •

edited

Loading

jyn514 Jan 11, 2021 •

edited

Loading

de-vri-es commented Nov 18, 2020 •

edited

Loading