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

Rework test error handling #11028

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Rework test error handling #11028

merged 1 commit into from
Aug 31, 2022

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Aug 28, 2022

This reworks how errors are handled when running tests and benchmarks. There were some cases where Cargo was eating the actual error and not displaying it. For example, if a test process fails to launch, it only displayed the could not execute process message, but didn't explain why it failed to execute. This fixes it to ensure that the full error chain is displayed.

This also tries to simplify how the errors are handled, and makes them more uniform across test and bench, and with doctests.

This also changes the --no-fail-fast behavior to report errors as they happen instead of grouped at the end (and prints a summary at the end). This helps to make it clearer when a nonstandard error happens. For example, before:

     Running tests/t1.rs (target/debug/deps/t1-bb449dfa37379ba1)

running 1 test
     Running tests/t2.rs (target/debug/deps/t2-1770ae8367bc97ce)

running 1 test
test bar ... FAILED

failures:

---- bar stdout ----
thread 'bar' panicked at 'y', tests/t2.rs:3:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    bar

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed.

Caused by:
  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t2-1770ae8367bc97ce` (exit status: 101)

and the changes to that are:

@@ -1,6 +1,10 @@
      Running tests/t1.rs (target/debug/deps/t1-bb449dfa37379ba1)

 running 1 test
+error: test failed, to rerun pass `--test t1`
+
+Caused by:
+  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
      Running tests/t2.rs (target/debug/deps/t2-1770ae8367bc97ce)

 running 1 test
@@ -18,8 +22,7 @@

 test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

-error: test failed.
-
-Caused by:
-  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
-  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t2-1770ae8367bc97ce` (exit status: 101)
+error: test failed, to rerun pass `--test t2`
+error: 2 targets failed:
+    `--test t1`
+    `--test t2`

In the first example, when it says Running tests/t1.rs, there is no error message displayed until after all the tests finish, and that error message is not associated with the original test. This also includes the "to rerun" hint with --no-fail-fast.

@rust-highfive
Copy link

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I do love this patch. The code is clearer, and also more docs are coming with this PR, not to mentioned tests themselves are more robust.

Thank you for posting this pull request!

@weihanglo
Copy link
Member

weihanglo commented Aug 30, 2022

Although this changes the output of cargo a bit, tools depend on the old output format should be rare. I will move forward and merge it.

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 30, 2022

📌 Commit 23735d4 has been approved by weihanglo

It is now in the queue for this repository.

@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 Aug 30, 2022
@bors
Copy link
Collaborator

bors commented Aug 30, 2022

⌛ Testing commit 23735d4 with merge d705fb3...

@bors
Copy link
Collaborator

bors commented Aug 31, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing d705fb3 to master...

@bors bors merged commit d705fb3 into rust-lang:master Aug 31, 2022
weihanglo added a commit to rust-lang/rust that referenced this pull request Sep 5, 2022
8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e
2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000
- Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511)
- refactor(cli): Lazy load config (rust-lang/cargo#11029)
- chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040)
- Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019)
- Add names to CI jobs (rust-lang/cargo#11039)
- Rework test error handling (rust-lang/cargo#11028)
- Very slight `cargo add` documentation improvements (rust-lang/cargo#11033)
- Update compiling requirements. (rust-lang/cargo#11030)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2022
Update cargo

8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e
2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000
- Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511)
- refactor(cli): Lazy load config (rust-lang/cargo#11029)
- chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040)
- Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019)
- Add names to CI jobs (rust-lang/cargo#11039)
- Rework test error handling (rust-lang/cargo#11028)
- Very slight `cargo add` documentation improvements (rust-lang/cargo#11033)
- Update compiling requirements. (rust-lang/cargo#11030)
@ehuss ehuss added this to the 1.65.0 milestone Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants