-
Notifications
You must be signed in to change notification settings - Fork 32
WIP:feat/notebook tests #255
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
base: main
Are you sure you want to change the base?
Changes from all commits
151a1d1
8b80f80
d78e11b
021cf78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| #!/usr/bin/env python | ||
| """Run all notebooks in tests/notebooks/ and report pass/fail.""" | ||
|
|
||
| import shutil | ||
| import subprocess | ||
| import sys | ||
| import tempfile | ||
| import time | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| NOTEBOOKS_DIR = Path(__file__).parent.parent / "docs/contents/notebooks" | ||
| BLACKLIST = ["extopt", "ir_spectrum", "ml_properties"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are they blacklisted?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
|
|
||
| def run_notebook(nb: Path) -> tuple[bool, float, str]: | ||
| """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please complete the docstring. |
||
| Helper function that runs a specific notebook and reports pass/fail. | ||
| Parameters | ||
| ---------- | ||
| nb | ||
|
|
||
| Returns | ||
| ------- | ||
|
|
||
| """ | ||
| start = time.monotonic() | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Describe in detail what you are doing here:
Where do make sure |
||
| nb_copy = Path(tmp) / nb.name | ||
| shutil.copy2(nb, nb_copy) | ||
| result = subprocess.run( | ||
| [ | ||
| "jupyter", | ||
| "nbconvert", | ||
| "--execute", | ||
| "--to", | ||
| "notebook", | ||
| "--ExecutePreprocessor.timeout=1800", | ||
| "--output", | ||
| nb.name, | ||
| "--output-dir", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess |
||
| tmp, | ||
| str(nb_copy), | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=tmp, | ||
| ) | ||
| elapsed = time.monotonic() - start | ||
| return result.returncode == 0, elapsed, result.stdout + result.stderr | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea with the boolean! |
||
|
|
||
|
|
||
| @pytest.mark.notebooks | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. notebooks also always run orca so I guess |
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in |
||
| 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] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. I think @haneug already wrote a custom summary report for one type of tests that we have. Maybe he can help.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if not notebooks: | ||
| print(f"No valid notebooks found in {NOTEBOOKS_DIR}") | ||
| sys.exit(1) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
|
|
||
| results: list[tuple[str, bool, float]] = [] | ||
| for nb in notebooks: | ||
| print(f" running {nb.stem} ...", end="", flush=True) | ||
| ok, elapsed, output = run_notebook(nb) | ||
| status = "PASS" if ok else "FAIL" | ||
| print(f" {status} ({elapsed:.1f}s)") | ||
| if not ok: | ||
| print(output) | ||
| results.append((nb.name, ok, elapsed)) | ||
|
|
||
| failures = [name for name, ok, _ in results if not ok] | ||
| print(f"{len(notebooks) - len(failures)}/{len(notebooks)} notebooks passed") | ||
| if failures: | ||
| print("Failed:") | ||
| for name in failures: | ||
| print(f" - {name}") | ||
| sys.exit(1) | ||
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.
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?
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.
No, sounds good!