oconnor663 / shared_child.rs

a wrapper around std::process::Child that lets multiple threads wait or kill at once
MIT License
39 stars 7 forks source link

tests::test_into_inner_after_wait test fail with Rust 1.72.0 and newer #23

Closed decathorpe closed 1 year ago

decathorpe commented 1 year ago

We maintain a package for this crate in Fedora Linux, and I noticed that the package now fails to build since we got the upgrade to Rust 1.72.0. This is the output from cargo test --release:

running 10 tests
test tests::test_into_inner_after_wait ... FAILED
test tests::test_into_inner_before_wait ... ok
test tests::test_kill ... ok
test tests::test_takes ... ok
test tests::test_try_wait ... ok
test tests::test_wait ... ok
test tests::test_waitid_after_exit_doesnt_hang ... ok
test tests::test_many_waiters ... ok
test tests::test_new ... ok
test unix::tests::test_send_signal ... ok
failures:
---- tests::test_into_inner_after_wait stdout ----
thread 'tests::test_into_inner_after_wait' panicked at 'called `Result::unwrap_err()` on an `Ok` value: ()', src/lib.rs:404:37
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
    tests::test_into_inner_after_wait
test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.11s
error: test failed, to rerun pass `--lib`
error: 1 target failed:
    `--lib`

There is a suspicious item in the Rust 1.72.0 release notes (https://github.com/rust-lang/rust/releases/tag/1.72.0):

Return Ok on kill if process has already exited

oconnor663 commented 1 year ago

Wow, I hadn't noticed that change, thank you. I agree that the new kill behavior is better.

I just pushed 80010627fa871cc9f469861a8c6675b6d6adc16a to remove that check from the test. Do you need a release with this fix, or is it enough for it to be on master?

decathorpe commented 1 year ago

I don't need a new version, having confirmation that skipping this test is a safe thing to do is enough for now. Thank you!