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

error in test execution #21

Closed
Nilsonfsilva opened this issue Dec 24, 2022 · 9 comments
Closed

error in test execution #21

Nilsonfsilva opened this issue Dec 24, 2022 · 9 comments

Comments

@Nilsonfsilva
Copy link

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

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

Nilsonfsilva commented Jan 3, 2023

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

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

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

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

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

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

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

@Roughsketch
Copy link
Owner

Closing as resolved

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