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

Document using a Mutex to sequentalise tests #43155

Open
nrc opened this Issue Jul 10, 2017 · 25 comments

Comments

Projects
None yet
@nrc
Copy link
Member

nrc commented Jul 10, 2017

cc #42684

One can use a static Mutex to sequentialise tests, but this is not documented, afaik.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 10, 2017

I guess the trick is this:

use std::sync::Mutex;
#[macro_use] extern crate lazy_static;

lazy_static! {
    static ref TEST_MUTEX: Mutex<()> = Mutex::new(());
}

macro_rules! test {
    (fn $name:ident() $body:block) => {
        #[test]
        fn $name() {
            let _guard = $crate::TEST_MUTEX.lock().unwrap();
            $body
        }
    }
}

test! { fn one() { println!("one"); } }
test! { fn two() { println!("two"); } }

Where the preamble could certainly be put in a support crate.

@kriomant

This comment has been minimized.

Copy link

kriomant commented Jul 15, 2017

This approach has a problem: when one test fails with panic, remaining ones fail with PoisonError.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 15, 2017

Hmm, good point. I suppose an implementation like this can fix that:

macro_rules! test {
    (fn $name:ident() $body:block) => {
        #[test]
        fn $name() {
            let guard = $crate::TEST_MUTEX.lock().unwrap();
            if let Err(e) = panic::catch_unwind(|| { $body }) {
                drop(guard);
                panic::resume_unwind(e);
            }
        }
    }
}

Another reason to put this in a support crate (or the standard test harness) to avoid this and likely other traps.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 15, 2017

You don't need to do anything with catch_unwind - just don't unwrap the result.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 15, 2017

@sfackler Does a poisoned mutex still... mutex?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 15, 2017

Yes - the guard object is still inside of the error - https://doc.rust-lang.org/std/sync/struct.PoisonError.html#method.into_inner

@kriomant

This comment has been minimized.

Copy link

kriomant commented Jul 16, 2017

Yes, problem can be worked around using PoisonError::into_inner. However, trick becomes verbose and convoluted, we really need something built-in to avoid it.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 16, 2017

I agree in general (that's why I made https://docs.rs/antidote), but here, we're not accessing anything in the mutex so you can just save off the Result<MutexGuard> in the guard variable and not worry about it at all.

@marmistrz

This comment has been minimized.

Copy link

marmistrz commented Jul 31, 2017

I couldn't agree more that we need something built-in. I tried adding the Mutex to my own tests, and this did not seem to work anyway. https://github.com/marmistrz/randmockery/blob/mutex/tests/test.rs

Setting RUST_TEST_THREADS=1 does the trick. Maybe it would simply be enough to allow setting this in Cargo.toml?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 31, 2017

@marmistrz let _ = mutex.lock() immediately discards the guard and unlocks the mutex. let _guard = mutex.lock() will do the right thing.

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Oct 28, 2017

I think I'm failing to understand why someone couldn't just use --test-threads=1 when running tests if they want them to be run in serial. I'm having trouble understanding why this is something we need to document

@marmistrz

This comment has been minimized.

Copy link

marmistrz commented Oct 28, 2017

@frewsxcv because the canonical, obvious way to run tests is cargo test. If someone who doesn't know the internals of the project wants to run the tests, that's what they'll do. Besides cargo test --test-threads=1 is much too long to write.

And, possibly, only some tests need mutual exclusion (e.g. because they use some single, problematic POSIX API, like it's in nix in case of fork/wait) and the others can safely run in parallel.

@steveklabnik steveklabnik added the P-low label Oct 31, 2017

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 31, 2017

Triage: marking as p-low. Seems like it's worth documenting, but not going to get to this any time soon.

@MasonRemaley

This comment has been minimized.

Copy link

MasonRemaley commented Nov 7, 2017

I just ran into this when writing tests for some OpenGL context creation code--this solution isn't possible in doctests since they don't share state, correct?

Worst case scenario I can just document that my library can only be tested via cargo test -- --test-threads 1 or move the doctests to normal tests, but the doc tests are pretty convenient and I'd rather the user not have to know that sort of thing if there's some other way to make this work.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 7, 2017

Each doctest is a separate process, so I'd expect there to be less issues with them running in parallel.

@MasonRemaley

This comment has been minimized.

Copy link

MasonRemaley commented Nov 14, 2017

It did cause an issue in my case, because my tests interact with the platform's windowing system which is shared between all processes.

I realized though that I have to have a note explaining this anyway though since other apps running on someone's machine could also mess with the windowing system, so I guess just documenting you need to use cargo test -- --test-threads 1 alongside that isn't that bad.

@glennpratt

This comment has been minimized.

Copy link

glennpratt commented Aug 25, 2018

I'm hitting this testing functions that parse environment variables to change various behaviors. A mutex would be really noisy to spread into all my tests when really just a few tests need to either run in sequence or run in a fork. Either way is quite a bit of boilerplate.

I looked and it seems Rust didn't have many tests for std::env, which made me wonder if this was the reason (or maybe I just didn't look hard enough). In other languages I'd probably fork and pass the value back for assertions, but I gather there are concerns with forking from the test runner. And of course being able to serialize the result and pass it back is a fair amount of boilerplate in Rust just for a test case.

@0xpr03

This comment has been minimized.

Copy link

0xpr03 commented Sep 4, 2018

Hitting the same problem here.
Database functions for which I'm currently creating virtual tables and force a connection hand-over instead of the normal pool approach, so I can test against this.
Hit a wall now trying to make parallel statements, leading to a tricky situation of borrowck vs multiple statements when trying to work with parallel tests.
This also prevents the pool cache for statements in my case.
blackbeam/rust-mysql-simple#154 (comment)

I'll have to re-parse my data multiple times, load everything into memory, disable parallel tests or drop test altogether.

@jeff-at-dwelo

This comment has been minimized.

Copy link

jeff-at-dwelo commented Dec 11, 2018

Hitting this issue trying to write integration tests against an MQTT server that does not like clients using the same credentials to open multiple connections. Having that macro boilerplate at the stop of my tests is super gross and apparently still failure prone.

I don't understand. We had a working PR into rust-lang to fix this in code back in 1.20, and it was summarily dropped. What happened at that meeting?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 8, 2019

I don't understand. We had a working PR into rust-lang to fix this in code back in 1.20, and it was summarily dropped. What happened at that meeting?

As the issue says, since this is possible without a new language feature, the new feature was not accepted.

@0xpr03

This comment has been minimized.

Copy link

0xpr03 commented Jan 8, 2019

As the issue says, since this is possible without a new language feature, the new feature was not accepted.

In the light of recent calls to stop adding feature over feature to rust it makes sense.
Edit: Welp it's already the "document this" issue..
I'm not sure but just documenting a possible solution for this should already help.

@marmistrz

This comment has been minimized.

Copy link

marmistrz commented Jan 9, 2019

This definitely doesn't belong to the language itself. How about adding a field in Cargo.toml which would have the same effect as --test-threads=1?

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Jan 10, 2019

@marmistrz It sounds like one goal of this hypothetical feature is to flag certain tests as a non-parallelizable, while the rest are. In which case it'd likely need to be some sort of attribute on an individual testcase or module

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Jan 10, 2019

lol, also I just realized I linked to a comment thinking it was someone else, but it's actually you 🙈

@0xpr03

This comment has been minimized.

Copy link

0xpr03 commented Jan 10, 2019

This definitely doesn't belong to the language itself. How about adding a field in Cargo.toml which would have the same effect as --test-threads=1?

That is a pretty nice idea and would help a lot.
I'm not sure but is it possible to create a configuration in cargo to let run xyz sequential and everything else parallel?
I've always had 3-4 tests which you can't run parallel (DB, global system calls like pulse audio etc) but all others can. So being able to configure this would a) let a new contributor just go ahead with cargo test (less ways to fail) and b) improve speed as not everything has to run sequential.

Edit: Of course there's always the possibility to define a test.sh, mark all sequentials as #[ignore] and define test.sh such that you first run a cargo test, then a cargo test --test-threads=1 followed by all sequential things. I think the downsides are obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment