Roughsketch / imagesize

Quickly probe the size of various image formats without reading the entire file.
MIT License
57 stars 12 forks source link

error in test execution #21

Closed Nilsonfsilva closed 1 year ago

Nilsonfsilva commented 1 year ago

Hey! imagesize is about to be part of the debian repositories.

However, there is a situation I need to discuss with you. You delete the "test" directory where the files to run the tests are stored. when compiling the package it gives this error:

running 1 test bmp_test test ... FAIL flaws: ---- bmp_test stdout ---- thread 'bmp_test' panicked on 'called Result::unwrap() on an Err value: IoError(Os { code: 2, kind: NotFound, message: "No such file or directory" })', tests/ bmp.rs:6:41

Would there be a way to resolve this?

Thanks!

Roughsketch commented 1 year ago

Sorry, I was away on vacation and missed this while I was gone.

Can you tell me how you are compiling the package? If it's possible to not package the tests or have them not run in certain scenarios I can add that if I know how to reproduce your issue.

Nilsonfsilva commented 1 year ago

Ola!

Sorry, I was away on vacation and missed this while I was gone. Don't worry, I understand the holiday rush!

Can you tell me how you are compiling the package? If it's possible to not package the tests or have them not run in certain scenarios I can add that if I know how to reproduce your issue.

First of all, it's a pleasure to bring your project to Debian!

Let's get down to business: In Debian, the packaging team is divided into teams according to the programming language. Each team develops tools with the aim of making the packaging process increasingly automated.

The tool we use in rust, extracts all information from packages in crates.io.

When we run the packaging tool on your project, it already downloads excluding the "test" directory. I believe this information comes from Cargo.toml

Note that you are deleting. https://github.com/Roughsketch/imagesize/blob/master/Cargo.toml

Since the process is fully automated, I cannot do it manually.

Wouldn't the solution be to remove that "delete" tag?

Thanks!

Roughsketch commented 1 year ago

I think removing that line wouldn't be good because it would then include all the test images in the build.

If there is no need for running tests on your end then I think the solution would be to also exclude the tests directory. That's where the code throwing an error is and it is not required for actually compiling the library. Renaming that directory locally so it isn't found runs fine on my end as well so should be good.

If you need tests for your repo then I'm not sure what the best course of action is because I don't want to package images with the distribution for anyone who uses the crate.

Roughsketch commented 1 year ago

I've made #22 which should address this. If you're able to test off that branch let me know if it works. If it requires a package/publish, then I'll likely try to get #20 merged at the same time.

Nilsonfsilva commented 1 year ago

I think this tag exclude = ["test/*"] doesn't make sense anymore. I think it should be removed.

Another thing, I advise you to keep the tests in your project.

Debian works with autopkgtests, periodically it runs the tests to verify that some release doesn't break your code.

Once that's done, you can merge changes from the fix/tests branch to the/master branch and release a new version.

also don't forget to update your code on crates.io. Because that's where the Debian tool pulls the information from.

Roughsketch commented 1 year ago

I really think that adding lots of image files from random sources to a cargo package isn't a good thing to do. This would increase the impact of installing the crate significantly for everyone (~25KB -> ~12MB with current tests, certainly larger in the future) while only being used rarely. Not to mention that with wide distribution licensing for test images becomes more important to keep track of.

The tests themselves are still in the repo and are set to run on PRs even after this change. The image specific tests just won't be able to be run from the package alone. Doc tests are still active and can be run from the package even if the tests directory is removed.

Are there any other packages that bundle test files like this to use as reference? I'm basing this off a few other image-related crates which explicitly don't bundle their test directories at all, for example the image crate's Cargo.toml explicitly removes all test directories and their data entirely which is what #22 is doing.

If the tests are an absolute requirement for some reason, then the only other thing I can think of is to strip the files of their image content and just keep headers around but that makes things harder to verify with external programs and requires manual work for every test. Certainly not something I want to have to support forever.

Nilsonfsilva commented 1 year ago

Hey! I understand your concerns.

Feel free to make any changes you feel are necessary to make the code more usable.

I already sent the packaging for approval as-is. I put a parameter that forces the tests even with errors.

If you change and merge into main branch, release a version and update on crates.io.

Thanks!

Roughsketch commented 1 year ago

@Nilsonfsilva 0.11.0 should be up now. Let me know if there are any other issues.

Roughsketch commented 1 year ago

Closing as resolved