WIP:feat/notebook tests#255
Conversation
…ectory for notebook tests
…es in notebook directory
timmyte
left a comment
timmyte
left a comment
There was a problem hiding this comment.
Looks good. It's definitely about time we also test the notebooks.
We should just polish the architecture a little.
|
|
||
|
|
||
| def run_notebook(nb: Path) -> tuple[bool, float, str]: | ||
| """ |
There was a problem hiding this comment.
Please complete the docstring.
|
|
||
| """ | ||
| start = time.monotonic() | ||
| with tempfile.TemporaryDirectory() as tmp: |
There was a problem hiding this comment.
Describe in detail what you are doing here:
- Where are notebooks executed?
- How are they executed?
- Why is there a timeout?
- Why does it need to copied over?
Where do make sure jupyter is installed?
| "--ExecutePreprocessor.timeout=1800", | ||
| "--output", | ||
| nb.name, | ||
| "--output-dir", |
There was a problem hiding this comment.
I guess --output-dir is redundant, as cwd=tmp.
| Function to iteratively test whether all notebooks (with exception to the ones to be excluded, which are defined in `BlACKLIST`) execute | ||
| without issues. In the case that a notebook execution fails,the function will print out which notebooks fail and the error message. | ||
| """ | ||
| notebooks = [nb for nb in NOTEBOOKS_DIR.glob("*.ipynb") if nb.stem not in BLACKLIST] |
There was a problem hiding this comment.
I think this should actually be a fixture.
Instead of one master function like here that acts as whole notebooks testsuite, I would boil this down to a single function, that takes a notebook, executes it and returns a flag if the notebook failed or not.
I would also keep the reporting to a minimum. Pytests captures the output anyway.
Though we should keep the printing of the timings. This is good for debugging and make sure out testsuite does not take too long ;)
I think @haneug already wrote a custom summary report for one type of tests that we have. Maybe he can help.
There was a problem hiding this comment.
I agree with the fixture / parametrize. I did add a custom error message when example tests fail although I do not remember much of this I am happy to discuss this.
| cwd=tmp, | ||
| ) | ||
| elapsed = time.monotonic() - start | ||
| return result.returncode == 0, elapsed, result.stdout + result.stderr |
There was a problem hiding this comment.
Good idea with the boolean!
| import pytest | ||
|
|
||
| NOTEBOOKS_DIR = Path(__file__).parent.parent / "docs/contents/notebooks" | ||
| BLACKLIST = ["extopt", "ir_spectrum", "ml_properties"] |
There was a problem hiding this comment.
Why are they blacklisted?
Please add a comment.
There was a problem hiding this comment.
Because they require additional external dependencies that cannot easily be set up by hand (extopt and ml_properties at least). This should be documented here. About ir_spectrum I am not sure?
|
|
||
| import pytest | ||
|
|
||
| NOTEBOOKS_DIR = Path(__file__).parent.parent / "docs/contents/notebooks" |
There was a problem hiding this comment.
Instead of hardcoding these complex paths, I think we should start adding symlinks to the repo. If we refactor the layout at some point, finding broken symlinks, is a peace of cake, but correcting the hardcoded paths is painful.
@haneug
Objections?
|
|
||
| import pytest | ||
|
|
||
| NOTEBOOKS_DIR = Path(__file__).parent.parent / "docs/contents/notebooks" |
| import pytest | ||
|
|
||
| NOTEBOOKS_DIR = Path(__file__).parent.parent / "docs/contents/notebooks" | ||
| BLACKLIST = ["extopt", "ir_spectrum", "ml_properties"] |
There was a problem hiding this comment.
Because they require additional external dependencies that cannot easily be set up by hand (extopt and ml_properties at least). This should be documented here. About ir_spectrum I am not sure?
| return result.returncode == 0, elapsed, result.stdout + result.stderr | ||
|
|
||
|
|
||
| @pytest.mark.notebooks |
There was a problem hiding this comment.
notebooks also always run orca so I guess @pytest.mark.orca is also appropriate here
| Function to iteratively test whether all notebooks (with exception to the ones to be excluded, which are defined in `BlACKLIST`) execute | ||
| without issues. In the case that a notebook execution fails,the function will print out which notebooks fail and the error message. | ||
| """ | ||
| notebooks = [nb for nb in NOTEBOOKS_DIR.glob("*.ipynb") if nb.stem not in BLACKLIST] |
There was a problem hiding this comment.
I agree with the fixture / parametrize. I did add a custom error message when example tests fail although I do not remember much of this I am happy to discuss this.
| @pytest.mark.notebooks | ||
| def test_notebooks() -> None: | ||
| """ | ||
| Function to iteratively test whether all notebooks (with exception to the ones to be excluded, which are defined in `BlACKLIST`) execute |
| notebooks = [nb for nb in NOTEBOOKS_DIR.glob("*.ipynb") if nb.stem not in BLACKLIST] | ||
| if not notebooks: | ||
| print(f"No valid notebooks found in {NOTEBOOKS_DIR}") | ||
| sys.exit(1) |
3 participants
Closes Issues
Closes #202
Description
Release Notes
(Project adheres to Keep a Changelog; Every entry should start in upper-case and end with
(#<pr-id>); Remove sections that don't apply)Added
test_notebooks(), which will execute all tutorial notebooks.(WIP:feat/notebook tests #255)notebookspytest tag fortest_notebooks()(WIP:feat/notebook tests #255)