taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

cargo llvm-cov nextest no multithreading for funtional tests? #258

Closed Dushistov closed 1 year ago

Dushistov commented 1 year ago

In crate I have "unit" tests and "functional" one.

// src/lib.rs
pub fn sleep(secs: u32) {
    std::thread::sleep(std::time::Duration::from_secs(secs.into()));
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_8() {
        sleep(8);
    }
}
// tests/functional_tests.rs
use crate_example_with_tests::*;

#[test]
fn test_30() {
    sleep(30);
}

Because of number of CPU on my machine is >=2 I expect that execution time cargo nextest is equal to time of the slowest test - test_30, and it works as expected:

❯ cargo nextest run
   Compiling crate_example_with_tests v0.1.0 (/home/evgeniy/bigdisk1/projects/rust-infra/crate_example_with_tests)
    Finished test [unoptimized + debuginfo] target(s) in 0.26s
    Starting 2 tests across 2 binaries
        PASS [   8.003s] crate_example_with_tests tests::test_8
        PASS [  30.003s] crate_example_with_tests::functional_tests test_30
------------
     Summary [  30.003s] 2 tests run: 2 passed, 0 skipped

but with llvm-cov it for some reasons waits all unit tests to pass, before starting functional tests:

❯ cargo llvm-cov nextest
   Compiling crate_example_with_tests v0.1.0 (/home/evgeniy/bigdisk1/projects/rust-infra/crate_example_with_tests)
    Finished test [unoptimized + debuginfo] target(s) in 0.33s
    Starting 2 tests across 2 binaries
        PASS [   8.003s] crate_example_with_tests tests::test_8
        PASS [  30.004s] crate_example_with_tests::functional_tests test_30
------------
     Summary [  38.008s] 2 tests run: 2 passed, 0 skipped

On real project all looks worse. Is any way to run functional and unit tests in parallel with llvm-cov?

taiki-e commented 1 year ago

cargo-llvm-cov limits the parallelism in test binaries to avoid the rustc bug. IIRC cargo-nextest does not respect RUST_TEST_THREADS (https://github.com/nextest-rs/nextest/issues/652), so in cargo llvm-cov nextest this is controlled using NEXTEST_TEST_THREADS environment variable. However, NEXTEST_TEST_THREADS seems to also limit overall parallelism.

@sunshowers: Is there a way to control only the number of threads created by test binaries without affecting the overall parallelism? (In other words, is there an equivalent option to RUST_TEST_THREADS?)

sunshowers commented 1 year ago

@taiki-e I think RUST_TEST_THREADS should work for that -- nextest doesn't do anything with it, and I think RUST_TEST_THREADS is read by libtest.

taiki-e commented 1 year ago

Hmm, at the time of https://github.com/nextest-rs/nextest/issues/652 reported, we already set RUST_TEST_THREADS=1, but it did not seem to be enough to avoid the rustc bug.

sunshowers commented 1 year ago

In that case I think something in rustc or llvm-cov might not support multiple test binaries being run in parallel.

Dushistov commented 1 year ago

So, may be just pass -C llvm-args=--instrprof-atomic-counter-update-all, and remove setting of RUST_TEST_THREADS to 1?

taiki-e commented 1 year ago

Yes, at least RUST_TEST_THREADS=1 can be replaced by -C llvm-args=--instrprof-atomic-counter-update-all. It is not clear yet if NEXTEST_TEST_THREADS can be removed.

taiki-e commented 1 year ago

If we can understand how cargo-nextest determines the number of tests to run simultaneously, we may also avoid the need to set NEXTEST_TEST_THREADS by using %Nm.

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program

“%Nm” expands out to the instrumented binary’s signature. When this pattern is specified, the runtime creates a pool of N raw profiles which are used for on-line profile merging. The runtime takes care of selecting a raw profile from the pool, locking it, and updating it before the program exits. If N is not specified (i.e the pattern is “%m”), it’s assumed that N = 1. The merge pool specifier can only occur once per filename pattern.

For a program such as the Lit testing tool which invokes other programs, it may be necessary to set LLVM_PROFILE_FILE for each invocation. The pattern strings “%p” or “%Nm” may help to avoid corruption due to concurrency. Note that “%p” is also a Lit token and needs to be escaped as “%%p”.

we currnetly sets %m (i.e., %1m)

https://github.com/taiki-e/cargo-llvm-cov/blob/752a20eb8c459c9d40e56fa36a34697127b709b3/src/main.rs#L201

sunshowers commented 1 year ago

Would it make sense to add a command to nextest showing the effective config along with its source?

taiki-e commented 1 year ago

Hmm, it may not actually be necessary to know the exact value specified in config. Here is a todo comment I added in #279:

https://github.com/taiki-e/cargo-llvm-cov/blob/ee3e3b844a3843a080906f2e73303df225224172/src/main.rs#L208-L217