search

agerada / AIgarMIC

AIgarMIC – machine-learning assisted agar dilution software
GNU General Public License v3.0
2 stars 2 forks source link

JOSS Review - CI/CD for non-asset tests and code test coverage #12

Closed gchure closed 3 weeks ago

gchure commented 1 month ago

Hi! I'm reviewing your paper for JOSS and will be opening issues as I go along through the checklist. This pertains to the test suite for the software.

I was able to install the software following your instructions and was able to run both the asset-based tests and the more minimal test suite. Is there a reason you do not have this running as a github action upon push to main? I understand downloading the assets may not be feasible going this route, but any amount of routine testing would be wise for promoting contributions.

Additionally, I think it would be useful to automatically generate some test coverage reports. The testing suite looks very good (although some documentation for the test functions would be nice!), but it's hard for me to see what fraction of the core software is covered by these tests. I have experience using codecov which I have enjoyed, though there are other options.

Neither of these points are blocking, but are "nice-to-haves".

agerada commented 1 month ago

Thank you @gchure for the helpful comments and feedback. I agree with all the points and will try to implement as many of them as I can. Much of the testing approach was driven by a desire to use actual assets (images, models) in the tests. Initially we did not have an assets flag for pytest. Since we now do, we should be fine to have a github action, albeit it will only be for the minimal testing suite.

I'm not familiar with codecov, but it looks very useful. I will look into whether it is compatible with assets, as that would be quite important for this project's code coverage. During development, I used pytest-cov, which is a project dependency. I had an issue with subprocesses not being measured for coverage, leading to the command line scripts tests not being included in the the report, but I think I have found a solution for this. I will explore this and add as a patch if it works.

gchure commented 1 month ago

Hi @agerada, I just submitted a PR to run CI/CD with all of the assets (see #13).

agerada commented 3 weeks ago

Hi @gchure. Just following up with an update on my progress with this issue.

All of these changes are in PR #23.

Thank you again for your helpful feedback and review.

gchure commented 3 weeks ago

Hi, #23 looks great. I think it would be useful to pipe the coverage report to a service like codecov so the coverage can be easily viewed (like this which I configured for my recent joss paper).

Feel free to veto that as desired, but I think that generating the coverage in a workflow is a little useless unless the report is accessible.

agerada commented 3 weeks ago

Thanks for forwarding your example – I do see the beneift now. I went ahead and set it up (#24), accessible here.

gchure commented 3 weeks ago

This is great. Thanks for handling that so swiftly. I'm satisfied with this and will close the issue and continue with the remaining bits of my review soon.