fix: resolve pytest return warning - #435
desken349-cmd wants to merge 1 commit into
Conversation
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: qBraid/pyqasm/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 Hey there! It looks like the changelog might need an update. Please take a moment to edit the
|
ryanhill1
left a comment
There was a problem hiding this comment.
Thanks for digging into this, and welcome. There's some context here that isn't visible from the file itself, so let me explain it — though I'm afraid the upshot is that we're going to close this one.
The return you removed is load-bearing: pytest-mpl needs it (B1, inline), and the warning it looks like it causes isn't actually emitted. So there's no issue to fix here, and the change would stop the test doing its job.
N1 — the description and commit message also mention a requirements.txt adding openpulse and tabulate, but the branch contains only the one-line test change. You shouldn't need one either: both are already declared in pyproject.toml — tabulate under the test and visualization extras, openpulse>=1.0.1 under pulse. If the tests wouldn't run for you, installing without extras is the likely cause; pip install -e ".[test,cli,pulse,visualization]" should sort it. A standalone requirements.txt would duplicate those extras and drift from them, so we'd rather not carry one.
If you'd like something real in this area, there is one worth doing. Nothing in the repo passes --mpl — CI runs plain pytest --cov=pyqasm tests/ (main.yml:163). The three mpl_image_compare tests still run and exercise mpl_draw(), but the image comparison itself never happens, so the baselines in tests/visualization/images/ are never checked. Wiring --mpl into CI, and refreshing whichever baselines have drifted since they were generated, would be a genuinely useful contribution — we'd be glad to review that.
| """ | ||
| fig = mpl_draw(qasm3) | ||
| return fig | ||
| assert fig is not None |
There was a problem hiding this comment.
B1 — pytest-mpl needs this return.
This test carries @pytest.mark.mpl_image_compare(baseline_dir="images", filename="misc2.png"). The plugin receives the figure to compare against tests/visualization/images/misc2.png from the test's return value, so removing the return hands it None:
this branch: E AttributeError: 'NoneType' object has no attribute 'savefig'
pytest_mpl/plugin.py:752
main: Error: Image dimensions did not match. <- comparison runs
(reproduce with pytest tests/visualization/test_mpl_draw.py --mpl)
Without --mpl the test still passes either way, which is why CI stays green — but the comparison can no longer work for anyone who does run it.
On the warning: it isn't being emitted. The full suite on the repo's pinned pytest (9.1.1) reports zero PytestReturnNotNoneWarning — pytest-mpl wraps the test, so pytest never sees the returned figure. The same goes for test_draw_bell (L178) and test_draw_misc_ops (L201), which also return fig and are correct as they stand.
Summary of Fixes
PytestReturnNotNoneWarningrule violation insidetests/visualization/test_mpl_draw.py. Converted an illegal test functionreturnstatement into a valid assertion handler (assert fig is not None), clearing test execution warning outputs.requirements.txtfile listing missing developer dependencies (openpulse,tabulate) to ensure future codebase clones can install and instantiate tests immediately.Validation Details
Summary of changes