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

Add run notebooks test #417

Merged

Conversation

bwheelz36
Copy link
Collaborator

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.

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (33b99ec) 98.57% compared to head (d4ed3a6) 98.57%.

📣 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bwheelz36 bwheelz36 requested review from fmfn and till-m May 7, 2023 06:51
Copy link
Member

@till-m till-m left a 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?

examples/constraints.ipynb Outdated Show resolved Hide resolved
@bwheelz36
Copy link
Collaborator Author

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.
The error you found (good catch!) is not in the tests, but me trying and failing to run the notebook and accidentally saving the result. I will push a fix for that in a minute.
The tests won't actually edit the existing notebooks (this is possible but painful with version control) but merely ensure that they can execute.

@bwheelz36 bwheelz36 mentioned this pull request May 8, 2023
Copy link
Member

@till-m till-m left a 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?)

@till-m
Copy link
Member

till-m commented May 9, 2023

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.

@bwheelz36
Copy link
Collaborator Author

" If a notebook is failing, is it obvious in which notebook the failure occured from the test logs?"

yes

"Minor nitpick, but is it necessary to change the notebooks in this PR? I anticipate that there might be some merge conflicts with #419."

Fair point - it wasn't necessary, but I'm going to just merge for now as it's less work :-)

@bwheelz36 bwheelz36 merged commit af1adc6 into bayesian-optimization:master May 9, 2023
6 checks passed
@bwheelz36 bwheelz36 deleted the add_run_notebooks_test branch May 9, 2023 09:45
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.

None yet

3 participants