sourcefrog / cargo-mutants

:zombie: Inject bugs and see if your tests catch them!
https://mutants.rs/
MIT License
483 stars 27 forks source link

Tests fail in unpacked crates.io crate tarball, due to missing testdata #355

Open sourcefrog opened 1 month ago

sourcefrog commented 1 month ago

Debian seems to build and run tests from the package uploaded to crates.io, which is reasonable enough. At the moment I exclude the testdata but this is a good reason not to. Anyhow, crates.io provides a kind of supplementary archive of the history of the project so it would be good if it's complete.

That would allow dropping https://sources.debian.org/patches/rust-cargo-mutants/23.10.0-1/disable-tests-missing-testdata.patch/

(cc @jelmer who seems to have kindly packaged, including finding this workaround, and uploaded it.)

However, this is more tricky than I expected because most of the testdata is comprised of example Rust trees and

https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields:

Regardless of whether exclude or include is specified, the following files are always excluded:

  • Any sub-packages will be skipped (any subdirectory that contains a Cargo.toml file).

We could somehow hide the testdata from cargo-package (e.g. renaming the files) but that seems like it would make development on the tree less straightforward.

We could potentially add a feature flag or other mechanism to let the tests pass if testdata is missing, and test from CI that this work.

Overall I'm not sure there's any much better option than manually cutting out the tests.

sourcefrog commented 1 month ago

Closing for now; can reopen if anyone has any better ideas. Maybe the simplest would be for Debian to fetch the github release tarball instead.

jelmer commented 1 month ago

👋

I did indeed package mutants (and some of the dependencies that were not yet packaged) for Debian - I use mutants regularly, it's one of the things I really liked in the Google3 ecosystem.

It would indeed be good if we could run more of the tests, or even if running tests from the crates.io archive works out of the box (whether that means running all tests or just some).

That way Debdan don't have to carry a patch (that will break as things change upstream), and it will make things easier for other packagers or people simply running from crates.io as well.

sourcefrog commented 1 month ago

I really liked it there too.

Maybe a good approach is: every test that wants testdata should just immediately succeed if the whole directory is missing. (I wish rust tests had a way to report "skipped" but I don't think they do at present.) In general that kind of pattern makes me concerned that some change will cause the test to succeed early even when it shouldn't, but after all we have cargo-mutants to catch that.

jelmer commented 1 month ago

Fetching from GitHub is technically possible but nontrivial - Debian has a fair amount of automation to package the 2000+ crates we package, and it's all based around crates.io today. Definitely something we could consider though.

sourcefrog commented 1 month ago

I started adding code to skip tests when testdata is missing, but, a large majority of the tests depend on the testdata, so it causes a lot of clutter. (In https://github.com/sourcefrog/cargo-mutants/compare/355-testdata?expand=1)

Maybe it would be better to just not run the tests from the Debian build? If most of them skip then it's not likely to find many problems, and I think nothing in the packaging is very likely to introduce a bug that's caught by this testsuite.

jelmer commented 1 month ago

Hm, I see what you mean - that does look quite messy. It'd be nice if there was the equivalent of raising TestSkipped() in Python from copy_of_testdata().

We can definitely just disable the tests in Debian if patching out the ones that rely on testdata becomes to cumbersome. I do prefer running as many of the tests as possible since we patch some of the dependencies to use whatever versions are in the Debian archive, rather than what is in Cargo.lock.

sourcefrog commented 1 month ago

OK, maybe a better alternative is: rename all the testdata/**/Cargo.toml to say Cargo_test.toml, then rename it back during copy_testdata.