-
Notifications
You must be signed in to change notification settings - Fork 7
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
[JOSS Review] Add automated tests in CI #37
Comments
Thanks for the comment. I think this requires a nbdev specific explanation as well. :) Tests in ciThis is something we have also discussed with our other reviewer. We had first included an explanation into the docs but have removed it again later for clarity. In short, nbdev doesn't separate the tests from the notebooks. When the CI builds the notebooks code it's also running the tests. From nbdev docs: "Everything that is not an exported cell is considered a test, so you should make sure your notebooks can all run smoothly (and fast) if you want to use this functionality as the CLI." The tests were written in notebooks right next to the doc and code. We're using ipytests to add tests in the notebooks and the assert keyword for simple tests. CoverageThere are a few issues when trying to run coverage with nbdev, but we're going to investigate how to do it and come up with at least a rough number. In addition to the technical difficulty to get the number, ipyannotator has a fair amount of ui-related code which is difficult to automatically test (eyeballing notebooks with rendered widgets is unreasonable effective) which makes the line coverage score even harder to interpret than it usually is. |
test coverage as reported by #41
|
This statement of mine was false as of PR #2 given ipyannotator/.github/workflows/release.yaml Lines 33 to 34 in cdcfba2
PR #40 improves on this by testing across all supported Python versions, and will resolve this Issue. 👍
90% coverage is rather low, but I will consider this as acceptable for publication. 👍 I would strongly suggest that you bring your coverage up. |
(There was a previous comment from me that I deleted that made an incorrect statement about there needing to be additional work. Please ignore that comment in any email notifications you received.) |
I'm closing the issue since the confusion about missing CI has been resolved and |
For the JOSS documentation check of openjournals/joss-reviews#4480
There are no automated tests (there are GitHub Action workflows for building and deploying the docs, but not for running the tests across all versions of Python supported). It seems that CI with some basic coverage of the API would greatly improve things.
For
v0.8.3
there are no instructions on how to run the tests in theREADME
(but they were added in for a future version in PR #34). It is additionally unclear what the test coverage of running the tests in the notebooks is, which would be important to know.The text was updated successfully, but these errors were encountered: