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.

Sign up for GitHub

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

Jump to bottom

Show the message in case of should_panic failure #61068

Closed
wants to merge 1 commit into from
Closed

Show the message in case of should_panic failure #61068

wants to merge 1 commit into from

Conversation

chansuke
Copy link
Contributor

@chansuke chansuke commented May 23, 2019 •
edited by varkor
Loading

This PR fixes a problem in #60790. Closes #60790.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@chansuke chansuke force-pushed the print-message-wish-should-panic branch from 7df5f6d to 0e53973 Compare May 23, 2019 06:47
@rust-highfive

This comment has been minimized.

Sign in to view
@Centril
Copy link
Contributor

Centril commented May 23, 2019

r? @varkor @bors rollup

@varkor

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2019
@Centril
Copy link
Contributor

Centril commented May 30, 2019

@varkor travis doesn't seem to pass?

@bors rollup-

varkor
varkor suggested changes May 30, 2019
View reviewed changes
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

Looks good after fixing the string issues. You can use ./x.py check to make sure everything type checks.

src/libtest/lib.rs Outdated
@@ -1543,7 +1543,7 @@ fn calc_result(desc: &TestDesc, task_result: Result<(), Box<dyn Any + Send>>) ->
}
}
_ if desc.allow_fail => TrAllowedFail,
_ => TrFailed,
_ => TrFailedMsg("test did not panic as expected"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ => TrFailedMsg("test did not panic as expected"),
_ => TrFailedMsg("test did not panic as expected".to_string()),

src/libtest/lib.rs Outdated
@@ -1923,7 +1923,7 @@ mod tests {
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, desc, tx, Concurrent::No);
let (_, res, _) = rx.recv().unwrap();
assert!(res == TrFailed);
assert!(res == TrFailedMsg("test did not panic as expected"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert!(res == TrFailedMsg("test did not panic as expected"));
assert!(res == TrFailedMsg("test did not panic as expected".to_string()));

@varkor
Copy link
Member

varkor commented May 30, 2019

@bors r-

Sorry, I didn't even spot that this wasn't passing.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 30, 2019
@chansuke chansuke force-pushed the print-message-wish-should-panic branch 2 times, most recently from 6236b80 to f6f5fea Compare June 15, 2019 09:15
@rust-highfive

This comment has been minimized.

Sign in to view
@chansuke chansuke force-pushed the print-message-wish-should-panic branch from f6f5fea to 7805691 Compare June 16, 2019 02:04
@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jun 16, 2019

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

@rust-highfive

This comment has been minimized.

Sign in to view
@chansuke chansuke force-pushed the print-message-wish-should-panic branch 2 times, most recently from bf41a98 to 26d3518 Compare June 18, 2019 16:23
@rust-highfive

This comment has been minimized.

Sign in to view
@chansuke chansuke force-pushed the print-message-wish-should-panic branch from 26d3518 to d68967d Compare June 19, 2019 04:36
@rust-highfive

This comment has been minimized.

Sign in to view
@varkor
Copy link
Member

varkor commented Jun 23, 2019

@chansuke: looks like a test is still failing?

@chansuke
Copy link
Contributor Author

@varkor Thank you for your comment. I'm trying to reproduce in my local environment(macOS Mojave) but only returns error below.
#59998
How can I debug this kind of error?

varkor
varkor reviewed Jul 22, 2019
View reviewed changes
src/libtest/lib.rs Outdated
@@ -1541,7 +1541,7 @@ fn calc_result(desc: &TestDesc, task_result: Result<(), Box<dyn Any + Send>>) ->
}
}
_ if desc.allow_fail => TrAllowedFail,
_ => TrFailed,
_ => TrFailedMsg("test did not panic as expected".to_string()),
Copy link
Member

@varkor varkor Jul 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I didn't check carefully enough. This should be:

(&ShouldPanic::Yes, Ok(())) => TrFailedMsg("test did not panic as expected".to_string()),
_ => TrFailed,

varkor
varkor reviewed Jul 22, 2019
View reviewed changes
src/libtest/tests.rs Outdated
@@ -146,7 +145,7 @@ fn test_should_panic_but_succeeds() {
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, desc, tx, Concurrent::No);
let (_, res, _) = rx.recv().unwrap();
assert!(res == TrFailed);
assert!(res == TrFailedMsg("test did not panic as expected".to_string()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert!(res == TrFailedMsg("test did not panic as expected".to_string()));
assert_eq!(res, TrFailedMsg("test did not panic as expected".to_string()));

@varkor
Copy link
Member

varkor commented Jul 22, 2019

How can I debug this kind of error?

If you look at the error in Travis, you'll see it says:

[01:47:28] diff output.json /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/libtest-json/libtest-json/libtest-json-output.json
[01:47:28] 5c5
[01:47:28] < { "type": "test", "name": "b", "event": "failed", "stdout": "thread 'main' panicked at 'assertion failed: false', f.rs:8:5\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.\n" }
[01:47:28] ---
[01:47:28] > { "type": "test", "name": "b", "event": "failed", "message": "test did not panic as expected" }

If you then look at the file src/test/run-make-fulldeps/libtest-json/f.rs, you'll see that this message should not have changed, because it's not #[should_panic]. So something was wrong with our change. In this case, the answer is clear once we look at the definition of ShouldPanic, which includes a ShouldPanic::Yes variant, which we weren't checking.

@totsteps
Copy link
Member

Ping from triage @chansuke any update on this? All checks all failing at the moment. Thanks

@chansuke
Copy link
Contributor Author

@gagan0723 Sorry for the late response, I will fix that now.

@chansuke chansuke force-pushed the print-message-wish-should-panic branch from d68967d to 5dfe71e Compare August 1, 2019 14:24
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@hdhoang
Copy link
Contributor

hdhoang commented Aug 9, 2019

ping from triage @chansuke, any update on this PR? thanks for your effort!

@chansuke chansuke force-pushed the print-message-wish-should-panic branch from e733944 to 333644a Compare August 10, 2019 16:13
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
2019-08-10T16:13:59.4257046Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-10T16:13:59.4446514Z ##[command]git config gc.auto 0
2019-08-10T16:13:59.4524744Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-10T16:13:59.4590995Z ##[command]git config --get-all http.proxy
2019-08-10T16:13:59.4724899Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/61068/merge:refs/remotes/pull/61068/merge
---
2019-08-10T16:14:34.1606487Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-10T16:14:34.1608479Z 
2019-08-10T16:14:34.1610530Z   git checkout -b <new-branch-name>
2019-08-10T16:14:34.1612285Z 
2019-08-10T16:14:34.1614429Z HEAD is now at d64457af3 Merge 333644a410eed975edb1641bd313444e6e476af3 into 6f70adcb18e5dc8df0672898a8404fd05a9c32cb
2019-08-10T16:14:34.1770836Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-10T16:14:34.1774216Z ==============================================================================
2019-08-10T16:14:34.1774278Z Task         : Bash
2019-08-10T16:14:34.1774323Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-10T17:19:22.0312813Z .................................................................................................... 1300/8868
2019-08-10T17:19:28.9083846Z .................................................................................................... 1400/8868
2019-08-10T17:19:35.4715689Z .................................................................................................... 1500/8868
2019-08-10T17:19:46.6149401Z ....................................................................................i............... 1600/8868
2019-08-10T17:19:54.7132068Z i................................................................................................... 1700/8868
2019-08-10T17:20:01.7619674Z ...........................................................................iiiii.................... 1800/8868
2019-08-10T17:20:25.0609253Z .................................................................................................... 2000/8868
2019-08-10T17:20:27.6678251Z .................................................................................................... 2100/8868
2019-08-10T17:20:30.5595665Z .................................................................................................... 2200/8868
2019-08-10T17:20:38.7741851Z .................................................................................................... 2300/8868
---
2019-08-10T17:24:35.0875872Z .................................................................................................... 5200/8868
2019-08-10T17:24:46.3524173Z .................................................................................................... 5300/8868
2019-08-10T17:24:54.1064543Z ....i............................................................................................... 5400/8868
2019-08-10T17:24:59.4982393Z .................................................................................................... 5500/8868
2019-08-10T17:25:12.5767664Z ...................................................................................................i 5600/8868
2019-08-10T17:25:27.6772497Z i...i..ii...........i............................................................................... 5700/8868
2019-08-10T17:25:44.7843974Z .................................................................................................... 5900/8868
2019-08-10T17:25:49.8285522Z .................................................................................................... 6000/8868
2019-08-10T17:25:49.8285522Z .................................................................................................... 6000/8868
2019-08-10T17:26:04.7970789Z i..ii............................................................................................... 6100/8868
2019-08-10T17:26:24.6505587Z ...........................................i........................................................ 6300/8868
2019-08-10T17:26:26.8914310Z .................................................................................................... 6400/8868
2019-08-10T17:26:29.5045585Z ...............i.................................................................................... 6500/8868
2019-08-10T17:26:34.2386994Z .................................................................................................... 6600/8868
---
2019-08-10T17:31:33.8365721Z  finished in 23.835
2019-08-10T17:31:33.8555592Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:31:34.0540985Z 
2019-08-10T17:31:34.0541801Z running 146 tests
2019-08-10T17:31:37.4871426Z i....iii......iii..iiii....i............................i..i................i....i.........ii.i.i..i 100/146
2019-08-10T17:31:39.4612555Z iii..............i.........iii.i......ii......
2019-08-10T17:31:39.4613159Z 
2019-08-10T17:31:39.4617965Z  finished in 5.606
2019-08-10T17:31:39.4815637Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:31:40.1568351Z 
---
2019-08-10T17:31:41.8488310Z  finished in 2.367
2019-08-10T17:31:41.8666230Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:31:42.0346797Z 
2019-08-10T17:31:42.0348014Z running 9 tests
2019-08-10T17:31:42.0350739Z iiiiiiiii
2019-08-10T17:31:42.0352790Z 
2019-08-10T17:31:42.0353700Z  finished in 0.168
2019-08-10T17:31:42.0539387Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:31:42.6615433Z 
---
2019-08-10T17:32:01.4534923Z  finished in 19.399
2019-08-10T17:32:01.4734813Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:32:01.6518121Z 
2019-08-10T17:32:01.6518878Z running 122 tests
2019-08-10T17:32:27.4125118Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....i..........iiii..........i...ii...i.......ii.i 100/122
2019-08-10T17:32:32.4165552Z .i.i......iii.i.....ii
2019-08-10T17:32:32.4167287Z 
2019-08-10T17:32:32.4172339Z  finished in 30.943
2019-08-10T17:32:32.4180538Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-10T17:32:32.4181174Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-08-10T17:43:16.4401765Z running 39 tests
2019-08-10T17:43:21.5216221Z ....................................F..
2019-08-10T17:43:21.5216726Z failures:
2019-08-10T17:43:21.5216932Z 
2019-08-10T17:43:21.5217716Z ---- tests::test_should_panic_but_succeeds stdout ----
2019-08-10T17:43:21.5218347Z thread 'tests::test_should_panic_but_succeeds' panicked at 'assertion failed: res == TrFailed', src/libtest/tests.rs:169:5
2019-08-10T17:43:21.5218705Z 
2019-08-10T17:43:21.5218841Z failures:
2019-08-10T17:43:21.5218997Z     tests::test_should_panic_but_succeeds
2019-08-10T17:43:21.5219119Z 
2019-08-10T17:43:21.5219119Z 
2019-08-10T17:43:21.5219261Z test result: FAILED. 38 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
2019-08-10T17:43:21.5219381Z 
2019-08-10T17:43:21.5229166Z error: test failed, to rerun pass '-p test --lib'
2019-08-10T17:43:21.5248490Z 
2019-08-10T17:43:21.5248490Z 
2019-08-10T17:43:21.5249503Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "2" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "test" "--" "--quiet"
2019-08-10T17:43:21.5249931Z 
2019-08-10T17:43:21.5250098Z 
2019-08-10T17:43:21.5256295Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-10T17:43:21.5256563Z Build completed unsuccessfully in 1:22:15
2019-08-10T17:43:21.5256563Z Build completed unsuccessfully in 1:22:15
2019-08-10T17:43:22.8478146Z ##[error]Bash exited with code '1'.
2019-08-10T17:43:22.8525481Z ##[section]Starting: Checkout
2019-08-10T17:43:22.8527483Z ==============================================================================
2019-08-10T17:43:22.8527547Z Task         : Get sources
2019-08-10T17:43:22.8527587Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@varkor
Copy link
Member

varkor commented Aug 11, 2019

@chansuke: I think you just need to add back in the change for the test (as suggested here) and everything should work.

@JohnCSimon
Copy link
Member

Pinging again from triage
@chansuke
please see the comment from @varkor
Thank you!

@wirelessringo
Copy link

Closing due to inactivity. Thank you @chansuke for your work on this PR. Please reopen this PR when you have a chance to make necessary changes, and be warned that pushing to this PR while it is closed prevents it from being reopened.

@wirelessringo wirelessringo 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 Aug 30, 2019
@varkor varkor mentioned this pull request Aug 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varkor varkor varkor requested changes

Assignees

@varkor varkor

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tests with #[should_panic] should print a message on failure
10 participants
@chansuke @rust-highfive @Centril @varkor @bors @totsteps @hdhoang @JohnCSimon @wirelessringo @Mark-Simulacrum