timeout_ms does not cause test to stop

[kevin-june]

When using timeout_ms, a long-running test doesn't seem to time out:

For example, the test:

#[test]
#[serial(timeout_ms = 50)]
fn test_oops_run_forever() {
    loop {
        thread::sleep(Duration::from_secs(1));
    }
}

will eventually print test tests::test_oops_run_forever has been running for over 60 seconds.

I would expect an expired timeout to panic and cause the test to fail.

[kevin-june]

Can this behavior be added to this repo's tests? Perhaps something like this would work:

#[test]
#[serial(timeout_ms = 500)]
#[should_panic]
fn timeout_can_panic() {
    loop {
        thread::sleep(Duration::from_secs(1));
    }
}

[kevin-june]

Here is another peculiar example of erroneous behavior - a test without a timeout can also get stuck:

#[cfg(test)]
mod tests {
    use serial_test::serial;
    use std::{thread, time::Duration};

    #[test]
    #[serial]
    fn test_a_thing() {}

    #[test]
    #[serial(timeout_ms = 50)]
    fn test_oops_run_forever() {
        loop {
            thread::sleep(Duration::from_secs(1));
        }
    }

    #[test]
    #[serial]
    fn test_another_thing() {}
}

prints:

running 3 tests
test tests::test_another_thing ... ok
test tests::test_a_thing has been running for over 60 seconds
test tests::test_oops_run_forever has been running for over 60 seconds

I hope that this helps!

[palfrey]

Yeah. So, it's original purpose was trying to catch scenarios where tests had gotten stuck because of things like that (or before I fixed that, weird scenarios with nested test calls). #68 got raised and #71 because originally the timeout was set per all tests, but it was hard to figure out where to set it because of ordering issues.

It's meant as a "timeout on the serial lock", not a "how long does this test take to run" timeout, but if the latter did exists, it would be a separate attribute.

I'm now leaning towards removing the entire thing given this confusion and I'm not sure how useful it is anymore. @Licenser You raised the previous issues with it. Would you be ok if I removed timeout_ms entirely, or do you still have a use case for it?

[palfrey]

Oooh, I've just seen https://docs.rs/ntest/latest/ntest/attr.timeout.html which may aid the original use case. Should work in combination with serial_test. Probably.

[Licenser]

I think removing it is fine, I never got around to making tests timeout to a per-test basis, sorry.

[palfrey]

I think removing it is fine, I never got around to making tests timeout to a per-test basis, sorry.

No worries. I should probably have spotted/flagged issues with this earlier. Also deleting is generally much easier to do anyways.

[palfrey]

#83 solves this by removing timeout_ms

[kevin-june]

#83 solves this by removing timeout_ms

That's understandable, and somewhat unfortunate for our use case. A per-test timeout is a useful feature.

Oooh, I've just seen https://docs.rs/ntest/latest/ntest/attr.timeout.html which may aid the original use case. Should work in combination with serial_test. Probably.

Ok, we will find an alternate solution.

Thanks!