-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add run notebooks test #417
Add run notebooks test #417
Conversation
update from main
Codecov ReportPatch and project coverage have no change.
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 8 8
Lines 560 560
Branches 79 79
=======================================
Hits 552 552
Misses 4 4
Partials 4 4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much -- looks good to me! The only question is, how do we know if it works?
Hi @till-m, we know it works because it has just run over this pull request - if you go into one of the test logs you will see that 'test_notebooks_run' has been executed and is passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this seems good to me.
The tests won't actually edit the existing notebooks (this is possible but painful with version control) but merely ensure that they can execute.
I think this is fine. PR looks good to me! 👍
If a notebook is failing, is it obvious in which notebook the failure occured from the test logs?
(I would merge, but I'm not sure if there is anything else you'd like to do?)
Minor nitpick, but is it necessary to change the notebooks in this PR? I anticipate that there might be some merge conflicts with #419. If the changes are not required (which I am not sure about) then having them also makes it harder to tell what exactly is happening in this PR. |
yes
Fair point - it wasn't necessary, but I'm going to just merge for now as it's less work :-) |
I've added a test that simply runs through all the notebooks in the examples folder. the test if fails if any notebook raises an error.