nextest-rs / datatest-stable

Data-driven tests on stable Rust
https://docs.rs/datatest-stable
Other
36 stars 6 forks source link

No coverage information generated #20

Closed zaneduffield closed 6 months ago

zaneduffield commented 8 months ago

I'm not sure if this is just some kind of user error, but I can't see to get coverage information generated for my datatests.

Coverage information is being generated, but it only seems to contain the hits from the non-datatest tests. When I clear the '*.profraw' files and run just the datatest tests, there is 0% coverage.

Initially I tried running cargo llvm-cov --html, then I tried following the steps for external tests but that didn't work either.

What might make my use-case a bit unusual is the fact that the datatest tests are in their own crate in the workspace, and I'm expecting coverage information to be generated for another crate in the workspace. I thought this structure could be part of the problem, so I tried replacing the datatest_stable::harness! macro with a main function that directly calls a function from the other crate, and then coverage information was successfully generated.

Any help on this issue would be greatly appreciated.

sunshowers commented 6 months ago

Hi, thanks for the report -- completely missed this earlier.

This is very strange. I got around to adding coverage for datatest-stable itself, and it seems to work fine: https://app.codecov.io/github/nextest-rs/datatest-stable/commit/ff0bbaf8923cdcb8621bafa459ad11589085845a/tree/src. (But this is within the same crate.)

datatest_stable::harness is just a main function that calls into your functions through a couple levels of indirection, so there really isn't anything that should be different.

Do you have a minimal reproduction example?

zaneduffield commented 6 months ago

@sunshowers I created a minimal example repo here: https://github.com/zaneduffield/datatest_coverage_issue

Please let me know if you can reproduce the issue with this.

sunshowers commented 6 months ago

Thanks for the example! Sadly I can't repro this at all. I get identical coverage results in both cases.

image

image

The results with your main and with the datatest-stable harness are completely identical.

This is with Rust 1.77.1 on Linux x86_64.

sunshowers commented 6 months ago

I realized that my cargo-llvm-cov was out of date (0.5.3). I updated it to the latest version, 0.6.9, and still see the same results.

zaneduffield commented 6 months ago

Sadly I can't repro this at all.

I thought that might be the case 😢. I just upgraded cargo-llvm-cov and rust to match your versions and I still have the issue. I am running on Windows, which would be the main remaining difference between our systems.

I'll close this here for now since it doesn't seem like the fault of datatest-stable. It's more likely to be an upstream issue with libtest-mimic (or some weird configuration issue on my system).

sunshowers commented 6 months ago

Ahh -- Windows is interesting! There might definitely be something weird going on there. Would definitely be curious to learn if you find out more.

zaneduffield commented 6 months ago

I think I worked out the problem. Right here, libtest_mimic::Conclusion::exit is called on the return value of libtest_mimic::run https://github.com/nextest-rs/datatest-stable/blob/719600dcbc6962cd2697df7543700df6d34d795d/src/runner.rs#L17

Internally, this function calls process::exit https://github.com/LukasKalbertodt/libtest-mimic/blob/d68ce73c7066a769e303b589cf79676c33c09495/src/lib.rs#L339C1-L350C6

From the documentation of process::exit

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread's stack will be run.

I have a feeling that (maybe just on Windows), something is depending on destructors being terminated on some thread.

When I return ExitCode from the main function, instead of calling libtest_mimic::Conclusion::exit, coverage works perfectly.

For example, if I use this as my main function I get no coverage

use std::{path::PathBuf, process::ExitCode};
use libtest_mimic::{Arguments, Trial};

fn main() {
    let args = Arguments::from_args();

    libtest_mimic::run(
        &args,
        vec![Trial::test("foo", || {
            call_fn_to_cover(&PathBuf::new()).unwrap();
            Ok(())
        })],
    ).exit();
}

but if I change it to use ExitCode, then coverage is generated properly:

use std::{path::PathBuf, process::ExitCode};
use libtest_mimic::{Arguments, Trial};

fn main() -> ExitCode {
    let args = Arguments::from_args();

    let result = libtest_mimic::run(
        &args,
        vec![Trial::test("foo", || {
            call_fn_to_cover(&PathBuf::new()).unwrap();
            Ok(())
        })],
    );

    if result.has_failed() {
        ExitCode::from(101)
    } else {
        ExitCode::SUCCESS
    }
}
sunshowers commented 6 months ago

Aha! Yes that would explain it, wouldn't it. Worth filing in libtest-mimic, and honestly might be a bug in Rust itself.

sunshowers commented 6 months ago

I've fixed this in 33d4725, planning to get a release out very soon.

sunshowers commented 6 months ago

Meanwhile could you try patching that in locally and ensuring it works?

zaneduffield commented 6 months ago

Meanwhile could you try patching that in locally and ensuring it works?

Yep, with this patch coverage is generated successfully.

[patch.crates-io]
datatest-stable = { git = "https://github.com/nextest-rs/datatest-stable/", rev = "33d4725463e839060c70254b0c879ad5e27a3cfb" }

Thanks for fixing this 😄.

Do you still think I should raise this with libtest-mimic? I guess the only thing they could be doing better is have a function that returns an ExitCode instance, but it would be a breaking change to have the existing Conclusion::exit function do that.

I wouldn't really know if this is worth raising with rust (or llvm).

sunshowers commented 6 months ago

Awesome, datatest-stable 0.2.5 should be out in a few.

I think it's absolutely worth raising with libtest-mimic, yeah. Other custom harnesses are going to run into this. Personally I'd consider deprecating Conclusion::exit, and adding a new exit_code method to Conclusion.

The MSRV for ExitCode is Rust 1.61 so it's been long enough.

As far as Rust or llvm goes, probably not, but I think it's worth getting this documented in llvm-cov. Windows is a relatively rare OS for Rust dev and std::process::exit seems to work fine on Linux, so this will be surprising to a lot of people I think.

Thanks again and great job on the investigation!

zaneduffield commented 6 months ago

FYI @sunshowers libtest-mimic 0.7.1 just added Conclusion::exit_code

sunshowers commented 6 months ago

FYI @sunshowers libtest-mimic 0.7.1 just added Conclusion::exit_code

Thanks. Needs LukasKalbertodt/libtest-mimic#40 to be fixed to use it.

sunshowers commented 6 months ago

@zaneduffield released datatest-stable 0.2.6 with libtest-mimic 0.7.2. Thanks.