nextest-rs / nextest

A next-generation test runner for Rust.
https://nexte.st
Apache License 2.0
2.19k stars 96 forks source link

Fail tests that leak pipes #15

Closed sunshowers closed 2 years ago

sunshowers commented 2 years ago

It is possible to write a test that ends up leaking a pipe (e.g. creates a process but doesn't terminate it). Currently, the test runner hangs on encountering such a test.

Instead, we should figure out a way to:

brkydnc commented 2 years ago

Hi, could you please give an example piece of code that causes this behavior? This is for understanding the problem better. Thanks in advance.

sunshowers commented 2 years ago

What you need is:

  1. A test starts a server-like executable capturing its output
  2. The test panics without closing down the server
  3. The server never exits and stays running in memory

In these cases nextest will wait around for the server process to exit rather than marking the test as LEAK and moving on.

Bauxitedev commented 2 years ago

I've run into this issue as well. Strangely enough, passing --no-capture to Nextest causes the tests to run normally. But if I remove the flag, the tests hang forever.

I also tried adding this to my .config/nextest.toml...

slow-timeout = {period = "30s", terminate-after = 2}

...but it still doesn't terminate the tests at all. (again, unless running with --no-capture, in which case it terminates properly)

This leads to an annoying Heisenbug where attempting to debug it, by passing --no-capture, causes the bug to disappear.

Bauxitedev commented 2 years ago

Update: I made a quick reproduction test of the issue.

use std::{
    process::{Command, Stdio},
};

#[test]
fn bug() {
    let mut cmd = Command::new("sleep");
    cmd.args(vec!["9999"]);
    let mut child = cmd.spawn().unwrap();
}

If you run this test with cargo nextest, it will get stuck forever, even if you specify a value for terminate-after in .config/nextest.toml: image

However, if you run cargo nextest --no-capture, it instantly finishes: image

This is especially annoying for running a server process in the test, since if the test panics, the test will never finish and you don't get feedback about whether or not the test failed.

sunshowers commented 2 years ago

Thanks for the further report. I agree that this should be a priority to fix given that even terminate-after isn't enough. Will look into it really soon.

sunshowers commented 2 years ago

I started playing around with how to do this -- it involves switching to an async executor so that we can select on timeouts and pipe reads at the same time, which I'm doing in #359.

sunshowers commented 2 years ago

@Bauxitedev this is now out as part of cargo-nextest 0.9.27.