Skip to content

Switch notebook tests to nbval#987

Merged
dwhswenson merged 2 commits into
openpathsampling:masterfrom
dwhswenson:nbval
Mar 5, 2021
Merged

Switch notebook tests to nbval#987
dwhswenson merged 2 commits into
openpathsampling:masterfrom
dwhswenson:nbval

Conversation

@dwhswenson

Copy link
Copy Markdown
Member

We currently run several notebooks as tests -- mostly system-scale smoke tests, though also some more unit-level testing. These tests are run using a tool that @jhprinz wrote, ipynbtest.

Unfortunately, ipynbtest hasn't been maintained for a quite a while. Additionally, after @jhprinz developed ipynbtest, another tool, nbval, became a widely used pytest plugin for using Jupyter notebooks in testing.

With no one to maintain ipynbtest, I think we need to switch to nbval. This PR does that. I'm sad about this, because I think ipynbtest is a nicer tool to use (I like the way it reports Markdown headings as a progress indicator while running.) However, this is now the only thing blocking us from testing on Python 3.8 and 3.9, so the time has come.

@codecov

codecov Bot commented Feb 28, 2021

Copy link
Copy Markdown

Codecov Report

Merging #987 (80f73c7) into master (d32c198) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #987   +/-   ##
=======================================
  Coverage   80.28%   80.28%           
=======================================
  Files         136      136           
  Lines       14468    14468           
=======================================
  Hits        11616    11616           
  Misses       2852     2852           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d32c198...80f73c7. Read the comment docs.

@dwhswenson

Copy link
Copy Markdown
Member Author

close/reopen here because it seems that we never recovered from a github actions outage this morning

@dwhswenson dwhswenson closed this Mar 1, 2021
@dwhswenson dwhswenson reopened this Mar 1, 2021
@dwhswenson

dwhswenson commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

This is ready for review and comment. I will leave it open for at least 72 hours, merging no earlier than Thu 04 Mar 15:00 GMT (16:00 local).

EDIT: Changed my mind from the 24 hour window -- this is a big enough change that I'll give a 72 hour window. It does involve a fundamental change to our testing procedure.

@sroet sroet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, 1 consistency question, please don't hang this PR on that.

Comment thread examples/ipynbtests.sh
date
ipynbtest.py --strict "test_pyemma.ipynb" || testfail=1
date
py.test --nbval --current-env \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was the -v consciously dropped here?

Suggested change
py.test --nbval --current-env \
py.test --nbval --current-env -v \

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Was the -v consciously dropped here?

Yes, this was intentional. The verbose output is not very useful with nbval, because each test is just named Cell 0, Cell 1, etc. (This is what I liked much more about ipynbtest and its parsing of the Markdown headings.) I added the -v for the slower notebooks, because I'm pretty sure that before too long I'll know exactly which cells are expected to take a while, and it'll be nice to get line-by-line updates for them. For fast-running notebooks, I didn't see a need for it.

@dwhswenson dwhswenson merged commit a09f2b3 into openpathsampling:master Mar 5, 2021
@dwhswenson dwhswenson deleted the nbval branch March 5, 2021 00:43
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants