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

Test snapshots contained in crates.io release #190

Open
weiznich opened this issue Jan 7, 2025 · 3 comments
Open

Test snapshots contained in crates.io release #190

weiznich opened this issue Jan 7, 2025 · 3 comments

Comments

@weiznich
Copy link

weiznich commented Jan 7, 2025

The colored 2.2.0 release contains insta snapshots as part of the crates.io release. Given that these snapshots are almost exclusively used for running tests it seems to be unnecessary to include them into the released package as they just increase the package size. Cargo allows you to declare which files to include/exclude in your Cargo.toml files.

See here for the contained files.

@spenserblack
Copy link
Collaborator

Thanks for the report! While it may seem unusual to see test assets get published, this allows tooling to pull down the crate from crates.io and runs tests on it. See this discussion for more details. The idea that published crates should be able to pass tests might be one of the reasons that cargo package and cargo publish don't automatically exclude the tests/ directory, for example.

If the test assets get exceptionally large and start to have a negative impact on pulling down the crate they should definitely be excluded. But right now their size is pretty negligible.

@weiznich
Copy link
Author

Thanks for the response. I agree with you that the test files are relatively small and additionally they are human readable, so they are not likely to be a large problem at all. Anyway I just want to mention that the linked discussion is only one opinion on this topic, there are other members in the community that promote the opinion that test files should always be excluded.

I personally tend to exclude such files as they make it harder to review the relevant parts of the code (there is just "more" to review) if you check your dependencies. That's especially a problem for binary files, so this only partially affects this particular case.

Anyway, feel free to close this issue if you feel that's not relevant, I got my answer.

@spenserblack
Copy link
Collaborator

spenserblack commented Jan 10, 2025

I personally tend to exclude such files as they make it harder to review the relevant parts of the code (there is just "more" to review)

I see this issue better now, thanks for explaining. I do think that it could be confusing to have .snap files in the src/ folder. This sounds like it could be at least partially resolved by moving snapshots to the tests/ folder, and I'd happily accept a PR that does that. To me, it feels like a reasonable middle ground between making the crate's code easier to review and also allowing tooling to test a published crate. What do you think?

Also, the it_works test could probably be changed to being an example, removing the snapshots.

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

No branches or pull requests

2 participants