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

Cover test files when publishing #35

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

anforowicz
Copy link
Contributor

This PR includes test files (less than 1kB currently) when publishing the crate. See below for the motivation.

This is a recent regression

My recent PR (#28) has broken an ability to run fdeflate tests on top of the version published to crates.io.

$ wget https://crates.io/api/v1/crates/fdeflate/0.3.5/download -O fdeflate-0.3.5.tar.gz
...
$ tar xvzf fdeflate-0.3.5.tar.gz
...
$ cd fdeflate-0.3.5/
$ cargo test
...
error: couldn't read `src/../tests/input-chunking-sensitivity-example1.zz`: No such file or directory (os error 2)
    --> src/decompress.rs:1295:62
     |
1295 |           let result = verify_no_sensitivity_to_input_chunking(include_bytes!(
     |  ______________________________________________________________^
1296 | |             "../tests/input-chunking-sensitivity-example1.zz"
1297 | |         ))
     | |_________^
     |
     = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)
...

Motivation for running tests against crates.io release

It seems that in some scenarios running tests against crates.io release is desirable:

  • Testing against original repo (e.g. against GitHub commit) may not be possible - around 10% of crates specify repository locations that are unreachable at time of testing. (source)
  • An internal testing system at Google uses tests downloaded from crates.io (rather than from the original repo like GitHub)
  • Crater (test system of the Rust compiler/toolchain) builds and runs crate tests - see https://users.rust-lang.org/t/what-to-include-when-publishing-a-crate/51992/5

IIUC the consensus at https://users.rust-lang.org/t/what-to-include-when-publishing-a-crate/51992/16 seems to be:

So if the crate's tests don't need large files, I'd prefer if they were included in the published package.
https://users.rust-lang.org/t/what-to-include-when-publishing-a-crate/51992/18

This PR seems reasonable

It seems to me that this PR is reasonable for fdeflate, but I am not 100% sure about this.

Alternatives:

  • Move the tests to tests/ directory:
    • Con: this excludes the tests from crates.io release
    • Pro: this doesn't package random binary files (I see no risk here, but people are quite suspicious after https://en.wikipedia.org/wiki/XZ_Utils_backdoor)
    • Pro: doesn't use up space on crates.io, bandwidth, space on consumers of the crate, etc.
  • Hide the tests behind #[cfg(...)]
    • Con: extra complexity from the new crate feature

Other notes:

  • AFAIU there is no official guidance to support cargo test in a package downloaded from crates.io
  • Test corpus of some crates (e.g. png) is quite big and would exceed the cap of package size. Therefore we can't include all tests for all crates.

@fintelia fintelia merged commit 464a904 into image-rs:main Oct 23, 2024
10 checks passed
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 this pull request may close these issues.

2 participants