time icon indicating copy to clipboard operation
time copied to clipboard

Omitting src/tests.rs from the package causes crater to skip this crate

Open saethlin opened this issue 2 years ago • 3 comments

I've been manually inspecting the crater logs from the most-downloaded crates, and I think a lot of people don't realize how many published crates are skipped.

This crate is skipped because its tests do not build based on the package that is uploaded to crates.io. Crater attempts to run cargo test --no-run and it gets:

[INFO] [stdout] error[E0583]: file not found for module `tests`
[INFO] [stdout]    --> src/lib.rs:316:1
[INFO] [stdout]     |
[INFO] [stdout] 316 | mod tests;
[INFO] [stdout]     | ^^^^^^^^^^

Full log here: https://crater-reports.s3.amazonaws.com/beta-1.58-1/1.57.0/reg/time-0.3.5/log.txt

This causes crate to classify this crate as build-fail (build compiler error), and thus it is not inspected for regressions between Rust releases. Though this crate's code may be well-tested by its widespread use, so whether you care about this is really up to you.

I suggest not excluding this file from the package. It would be nice to increase the precision of crater.

saethlin avatar Jan 08 '22 04:01 saethlin

First of all, thanks for what you're doing! Having crater cover more things is always a plus.

Given what you're doing, I presume you know some details about crater; apologies if this is not the case. I have a couple questions:

  • Is this repository (time-rs/time) included in crater runs? My understanding is that public GitHub repositories are largely included.
  • If it is included, does it execute? The time crate requires --all-features be passed to tests.
  • If not (and --all-features isn't passed), is there a way to request crater execute the tests a certain way? Similar to how docs.rs reads package.metadata.docs.rs in Cargo.toml, for example.

jhpratt avatar Jan 08 '22 05:01 jhpratt

I know a bit about crater.

Is this repository (time-rs/time) included in crater runs? My understanding is that public GitHub repositories are largely included.

It is not. As far as I can tell, the GitHub repositories that are the source of published crates are omitted.

If it is included, does it execute? The time crate requires --all-features be passed to tests. If not (and --all-features isn't passed), is there a way to request crater execute the tests a certain way? Similar to how docs.rs reads package.metadata.docs.rs in Cargo.toml, for example.

Phew, good thing you asked. I think this exact problem affects other crates as well, and it turns out I do not currently have a solution on hand.

Crater does not build or run tests with --all-features, and there currently does not exist a way to request that it do that. There isn't a way to make tests require features either, and the known workarounds mentioned in https://github.com/rust-lang/cargo/issues/2911 don't work for crater because path dependencies are stripped out of packaged crates, and doctests exist.

I think I'll toss something together into a crater PR, and keep this issue in sync when I do.

saethlin avatar Jan 08 '22 19:01 saethlin

I know a bit about crater.

More than me, which is basically what I was asking 🙂

the GitHub repositories that are the source of published crates are omitted

That certainly makes sense.

There isn't a way to make tests require features either

Well, that's certainly not ideal. I'll still likely end up making the change here, but having some metadata like there is for docs.rs would be wonderful.

jhpratt avatar Jan 09 '22 04:01 jhpratt

While having an option to request crater run with certain flags would be ideal, this is still not the case. As such, I implemented a neat workaround. If all features are not enabled, just spawn a child process with all the features enabled, failing the single test if the process as a whole fails. This is actually surprisingly easy to accomplish. I then updated doctests to work with the default feature set, so running cargo test by itself works!

I've removed the file exclusion from Cargo.toml, so time should be included in crater starting with the next release. Closing this issue as such.

jhpratt avatar Oct 25 '22 06:10 jhpratt