Skip to content
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

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

Closed
sourcefrog opened this issue Jun 9, 2024 · 7 comments · Fixed by #422
Closed

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

sourcefrog opened this issue Jun 9, 2024 · 7 comments · Fixed by #422
Assignees

Comments

@sourcefrog
Copy link
Owner

sourcefrog commented Jun 9, 2024

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 sourcefrog self-assigned this Jun 9, 2024
@sourcefrog sourcefrog changed the title Ship testdata in package upload Tests fail in unpacked crates.io crate tarball, due to missing testdata Jun 9, 2024
@sourcefrog sourcefrog linked a pull request Jun 9, 2024 that will close this issue
@sourcefrog sourcefrog added the wontfix This will not be worked on label Jun 9, 2024
@sourcefrog
Copy link
Owner Author

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
Copy link

jelmer commented Jun 9, 2024

👋

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
Copy link
Owner Author

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.

@sourcefrog sourcefrog reopened this Jun 9, 2024
@jelmer
Copy link

jelmer commented Jun 9, 2024

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 sourcefrog removed the wontfix This will not be worked on label Jun 9, 2024
sourcefrog added a commit that referenced this issue Jun 10, 2024
This isn't enough to let the tests pass in an unpacked package, but it
should allow e.g. building the book, and I think we might as well
include them.

This is related to #355 but won't fully fix it.
@sourcefrog
Copy link
Owner Author

sourcefrog commented Jun 10, 2024

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
Copy link

jelmer commented Jun 10, 2024

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants