Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
QuLogic commentedMay 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 forat 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.9PR #12307 and fix some errors." in 7.15. |
I only went back as far as 7.0.0, as due tomatplotlib#16263, we probably don'twant to be supporting all the way back to IPython 1.
I think we need to change the last line of assertproc.stdout.strip()==f"Out[1]: '{expected_backend}'" to something like assertproc.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 theendswith
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 failingtest_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.
1b7093d
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Err, that was a bit premature as tests weren't passing yet. |
Make the changes suggested by@ianthomas23, and also mark the ipythontests as using their backend. While the backend is already checked foravailability at the top of the respective files, that only checkswhether it can be imported, not whether it can be set as the Matplotlibbackend. Adding the marker causes our pytest configuration to actuallycheck 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 ipythontests as using their backend. While the backend is already checked foravailability at the top of the respective files, that only checkswhether it can be imported, not whether it can be set as the Matplotlibbackend. Adding the marker causes our pytest configuration to actuallycheck 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.xBackport 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