-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
TST: Fix tests with older versions of ipython #28205
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
Conversation
Ah, perhaps IPython 7.0.0 is a bit too old for Python 3.9 which was release Oct 5, 2020. Based on release dates, it seems like we should aim for at least IPython 7.19, released Oct 30, 2020? I didn't find any specific release notes about it except for "I’ve started to test on Python 3.9 PR #12307 and fix some errors." in 7.15. |
I only went back as far as 7.0.0, as due to matplotlib#16263, we probably don't want to be supporting all the way back to IPython 1.
I think we need to change the last line of assert proc.stdout.strip() == f"Out[1]: '{expected_backend}'" to something like assert proc.stdout.strip().endswith(f"'{expected_backend}'") as under some circumstances when I'm running old ipython but newish everything else I get (ignoring newlines)
|
Thanks for doing this, I didn't look very far back when I added these tests. Locally I've tried this branch on Python 3.9 on a mac (to run both the |
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.
I'm approving as is but it would good to include the endswith
change even though that line isn't yet changed by this PR and is only a problem when running tests locally rather than in CI.
For the failing test_backend_inline.py::test_ipynb
I would be inclined just to skip the test on Python 3.9. It relies on some of the Jupyter infrastructure working with a combination of old and new packages, and I don't think we should spend any time chasing that down when we have proved that IPython is OK here.
Err, that was a bit premature as tests weren't passing yet. |
Make the changes suggested by @ianthomas23, and also mark the ipython tests as using their backend. While the backend is already checked for availability at the top of the respective files, that only checks whether it can be imported, not whether it can be set as the Matplotlib backend. Adding the marker causes our pytest configuration to actually check and skip the test if unavailable (e.g., on Linux without `(WAYLAND_)DISPLAY` set fails to set an interactive backend).
🤦🏻 Sorry, I only saw the coverage failure and missed the real failure. |
Make the changes suggested by @ianthomas23, and also mark the ipython tests as using their backend. While the backend is already checked for availability at the top of the respective files, that only checks whether it can be imported, not whether it can be set as the Matplotlib backend. Adding the marker causes our pytest configuration to actually check and skip the test if unavailable (e.g., on Linux without `(WAYLAND_)DISPLAY` set fails to set an interactive backend).
TST: Followup corrections to #28205
…205-on-v3.9.x Backport PR #28205 on branch v3.9.x (TST: Fix tests with older versions of ipython)
PR summary
I only went back as far as 7.0.0, as due to #16263, we probably don't want to be supporting all the way back to IPython 1.
I believe that this only affected the test, as on IPython <8.24, we use the code from IPython anyway. But I've pinned the tests to IPython 7 so we should see if everything's good.
Closes #28202
PR checklist