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

Improve coverage testing and filesystem coverage #351

Merged
merged 14 commits into from
Jun 9, 2024

Conversation

JMicheli
Copy link
Collaborator

@JMicheli JMicheli commented Jun 6, 2024

This pull request is a bit of a grab-bag of testing improvements. I'll describe them in three categories: 1) code coverage tooling, 2) new testing data, 3) new tests.

Code coverage tooling

Since I've been making some tests additions, I thought it would be nice to start using some tools to track how that was going. I found cargo-llvm-cov and it seems perfect. It runs all tests and generates a file that a gutters extension in VSCode can use to give visual feedback on what lines of code are being tested.

To support this, I added a tasks.json file, and added a task that automatically runs the command to generate lcov.info (which I added to the .gitignore). I also added a setup task for good measure since I always find myself running that.

New testing data

I wanted to add some more file tests, so I added some new testing data alongside the other data in integration-tests (I would like to clean that up sometime and get it working again). Everything I added is public domain, and includes a webp image from Google, a cbz comic I got from a public domain comic archive, and a PDF copy of the rust book.

New tests

I added tests to various filesystem modules that used the aforementioned data to make sure various processing functions worked. I don't have total coverage in each module yet because certain things were harder to test than others. But it's a start!

I also added tests for the webp module, and I found that I had no issue passing when I loaded the data from the file as a Vec<u8> and passed it to the processor, but that when I used the function to open a path it would fail. I think there was something wrong with the way data was being loaded there, so I changed it to a simple fs::read(path) and it seemed like that did the trick. But maybe I misunderstood the reasoning for the way this was originally written - please take a look at that and confirm that what I've done works.

Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for tackling all of these testing improvements!!

I think there was something wrong with the way data was being loaded there, so I changed it to a simple fs::read(path) and it seemed like that did the trick. But maybe I misunderstood the reasoning for the way this was originally written - please take a look at that and confirm that what I've done works.

To be honest, I can't recall why I chose the image::io::Reader. If I had to guess, I likely assumed that the reader would have extra utilities baked in for facilitating image decoding/encoding. Your logic is sound, though, and I have no objections to keeping it as you made it 👍

core/src/filesystem/media/rar.rs Outdated Show resolved Hide resolved
@JMicheli
Copy link
Collaborator Author

JMicheli commented Jun 9, 2024

Addressed comments, ready to merge.

@aaronleopold aaronleopold merged commit efbae49 into stumpapp:develop Jun 9, 2024
3 checks passed
@JMicheli JMicheli deleted the filesystem-tests branch June 16, 2024 08:19
@aaronleopold aaronleopold mentioned this pull request Jul 8, 2024
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