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

libtest: Wait for test threads to exit after they report completion #81367

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Jan 25, 2021

Otherwise we can miss bugs where a test reports that it succeeded but then panics within a TLS destructor.

Example:

use std::thread::sleep;
use std::time::Duration;

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        sleep(Duration::from_secs(1));
        panic!()
    }
}

thread_local!(static FOO: Foo = Foo);

#[test]
pub fn test() {
    FOO.with(|_| {});
}

Before this fix, cargo test incorrectly reports success.

$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/panicking_test-85130fa46b54f758

running 1 test
test test ... ok

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

$ echo $?
0

After this fix, the failure is visible. (The entire process is aborted due to #24479.)

$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/panicking_test-76180625bc2ee3c9

running 1 test
thread 'test' panicked at 'explicit panic', src/main.rs:9:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
error: test failed, to rerun pass '--bin panicking-test'

Caused by:
  process didn't exit successfully: `/tmp/panicking-test/target/debug/deps/panicking_test-76180625bc2ee3c9 --nocapture` (signal: 6, SIGABRT: process abort signal)

$ echo $?
101

Otherwise we can miss bugs where a test reports that it succeeded but
then panics within a TLS destructor.

Signed-off-by: Anders Kaseorg <[email protected]>
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Jan 25, 2021
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! This is great.

@dtolnay
Copy link
Member

dtolnay commented Jan 25, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 25, 2021

📌 Commit 57c72ab has been approved by dtolnay

@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 Jan 25, 2021
@jonas-schievink
Copy link
Contributor

failed in #81385 (comment)

@bors r- rollup=iffy

@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 Jan 25, 2021
@andersk
Copy link
Contributor Author

andersk commented Jan 25, 2021

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', library/test/src/lib.rs:347:75

I guess that could happen if a test returns after timing out. Will fix.

This reduces the total complexity of checking timeouts from quadratic
to linear, and should also fix an unwrap of None on completion of an
already timed-out test.

Signed-off-by: Anders Kaseorg <[email protected]>
@andersk andersk requested a review from dtolnay January 25, 2021 20:27
@dtolnay
Copy link
Member

dtolnay commented Jan 25, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 25, 2021

📌 Commit b05788e has been approved by dtolnay

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2021
@bors
Copy link
Contributor

bors commented Jan 26, 2021

⌛ Testing commit b05788e with merge 1483e67...

@bors
Copy link
Contributor

bors commented Jan 26, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 1483e67 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 26, 2021
@bors bors merged commit 1483e67 into rust-lang:master Jan 26, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 26, 2021
@andersk andersk deleted the join-test-threads branch January 29, 2021 20:57
andersk added a commit to andersk/rust that referenced this pull request Feb 18, 2021
It is possible for different tests to collide to the same TestDesc
when macros are involved.  That is a bug, but it didn’t cause a panic
until rust-lang#81367.  For now, change the code to ignore this problem.

Fixes rust-lang#81852.

Signed-off-by: Anders Kaseorg <[email protected]>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 19, 2021
libtest: Fix unwrap panic on duplicate TestDesc

It is possible for different tests to collide to the same `TestDesc` when macros are involved. That is a bug, but it didn’t cause a panic until rust-lang#81367. For now, change the code to ignore this problem.

Fixes rust-lang#81852.

This will need to be applied to `beta` too.
cuviper pushed a commit to cuviper/rust that referenced this pull request Mar 10, 2021
It is possible for different tests to collide to the same TestDesc
when macros are involved.  That is a bug, but it didn’t cause a panic
until rust-lang#81367.  For now, change the code to ignore this problem.

Fixes rust-lang#81852.

Signed-off-by: Anders Kaseorg <[email protected]>
(cherry picked from commit 1605af0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants