assert_eq failure message easier to read #42541
Conversation
By having the left and right strings above and below on the same line it helps spot the difference between the two. E.g. thread 'tests::test_safe_filename' panicked at 'assertion failed: `(left == right)` left: `"-aandb--S123.html"` right: `"-aandb-S123.html"`', When the strings are both on the same line it take a lot longer to spot the difference. It is a small change but the small time savings add up with repetition. This helps Rust be an excellent language to write tests in.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
The test case for [00:52:03] failures:
[00:52:03]
[00:52:03] ---- [run-fail] run-fail/assert-eq-macro-panic.rs stdout ----
[00:52:03]
[00:52:03] error: error pattern 'assertion failed: `(left == right)` (left: `14`, right: `15`)' not found!
[00:52:03] status: exit code: 101
[00:52:03] command: /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-fail/assert-eq-macro-panic.stage2-x86_64-unknown-linux-gnu
[00:52:03] stdout:
[00:52:03] ------------------------------------------
[00:52:03]
[00:52:03] ------------------------------------------
[00:52:03] stderr:
[00:52:03] ------------------------------------------
[00:52:03] thread 'main' panicked at 'assertion failed: `(left == right)`
[00:52:03] left: `14`
[00:52:03] right: `15`', /checkout/src/test/run-fail/assert-eq-macro-panic.rs:14
[00:52:03] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:52:03]
[00:52:03] ------------------------------------------
[00:52:03]
[00:52:03] thread '[run-fail] run-fail/assert-eq-macro-panic.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2480
[00:52:03] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:52:03]
[00:52:03]
[00:52:03] failures:
[00:52:03] [run-fail] run-fail/assert-eq-macro-panic.rs
[00:52:03]
[00:52:03] test result: FAILED. 125 passed; 1 failed; 0 ignored; 0 measured; 0 filtered outBut the |
|
Might also be good to change the |
shepmaster
added
the
S-waiting-on-author
|
multiple error-pattern seems to work nicely 👍 . Ready for review. |
|
@alexcrichton is it looking ok now? |
|
Oops thanks for the ping @gilescope, turns out you're not the first to have this idea! Looks like this closes that issue so I'll update the PR description as well. Otherwise looks great to me, thanks! @bors: r+ |
|
📌 Commit 02969a7 has been approved by |
|
⌛ Testing commit 02969a7 with merge 2add77b... |
|
💔 Test failed - status-travis |
|
Legitimate failure. |
|
Cargo strikes again 😆 |
|
The fix for the cargo/tests/bench.rs is on gilescope/cargo revision 65ea42e8a847171e4f93fef6df5154aba7f832e3 (master branch). I'm unclear as to whether I should be creating a separate pull request for Cargo? Apologies - my git-foo seems to be lacking on submodules. Apologies for being a newbie in this respect. |
|
@gilescope Yes you need to submit a PR to cargo, wait for cargo to merge it, and then update the submodule here. You may see how this is handled in #41910 and rust-lang/cargo#4055. |
|
☔ The latest upstream changes (presumably #42676) made this pull request unmergeable. Please resolve the merge conflicts. |
Make test less brittle prior to assert_eq failure message format change PR required for rust-lang/rust#42541 to make assert_eq error message be multi-line. Before implementing this we need to make the current test less brittle. Not 100% clear on if I need the final [...] or not.
|
@alexcrichton good morning, hopefully second time luck with cargo! |
|
@bors: r+ |
|
📌 Commit 7acaf18 has been approved by |
|
@gilescope ah sorry there's no way to restart a travis build other than with a new commit, but I think this is ready to go, right? Want to remove the noop commits and I'll r+? |
|
My git-fu has certainly improved a bit! Ok, I think I've managed to scrub those two no-op commits. Please feel free to r+ |
|
@bors: r+ Thanks! |
|
📌 Commit 940d5ca has been approved by |
|
⌛ Testing commit 940d5ca with merge 5e696959cd5a3b42e58705aa5079a442c1f441d3... |
|
💔 Test failed - status-travis |
|
@bors retry network failure, I hope: [00:03:14] curl: (6) Could not resolve host: codeload.github.com |
|
@bors r=alexcrichton retry Didn't mean to close... |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 940d5ca has been approved by |
|
⌛ Testing commit 940d5ca with merge 22c905e1f145cbf7cbc188ad308a7bfc8d2a8d6e... |
|
💔 Test failed - status-travis |
|
macOS timed out, spurious. |
|
Hmm, Travis timed out at 3h but it was happily chugging away running the tests. Maybe it hit a slow / heavily loaded server? I guess at some point we need to split the build from running the tests so that we can parallelise the tests further. Then again speeding up the rust compiler would help these build times and might have some side benefits. Not sure I'm quite ready to take that on as my second PR. |
|
@gilescope no need to worry about that, some rust team member will just retry in these cases |
|
@bors retry OS X timed out |
assert_eq failure message easier to read By having the left and right strings aligned with one another it helps spot the difference between the two far quicker than if they are on the same line. E.g. Before: ``` thread 'tests::test_safe_filename' panicked at 'assertion failed: `(left == right)` left: `"-aandb--S123.html"` right: `"-aandb-S123.html"`', ``` After: ``` thread 'tests::test_safe_filename' panicked at 'assertion failed: `(left == right)` left: `"-aandb--S123.html"` right: `"-aandb-S123.html"`', ``` When the strings are both on the same line it take a lot longer to spot the difference. It is a small change but the small time savings add up with repetition. This would help Rust be an excellent language to write tests in out of the box. Closes #41615
|
☀️ Test successful - status-appveyor, status-travis |
|
Late to the game here, just wanted to say thanks for this. Thanks! |
By having the left and right strings aligned with one another it helps spot the difference between the two far quicker than if they are on the same line.
E.g.
Before:
After:
When the strings are both on the same line it take a lot longer to spot the difference. It is a small change but the small time savings add up with repetition. This would help Rust be an excellent language to write tests in out of the box.
Closes #41615