Skip to content

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

Merged
merged 1 commit into from
May 10, 2024

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented May 10, 2024

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

@QuLogic
Copy link
Member Author

QuLogic commented May 10, 2024

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.
@ianthomas23
Copy link
Member

I think we need to change the last line of lib/matplotlib/testing/__init__.py from

   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)

Out[1]: Installed osx event loop hook. 'MacOSX'

@ianthomas23
Copy link
Member

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 qt and osx tests) with progressively older ipython, and it works (as in the tests pass) for ipython >= 7.3.0 but fails for ipython < 7.3.0. Ipython 7.3.0 was released in February 2019 so pre-dates Python 3.9 by a good year and a half but still seems to work with it. So officially covering >= 7.19 seems good to me as it is rule-based on the Python 3.9 release date.

Copy link
Member

@ianthomas23 ianthomas23 left a 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.

@tacaswell tacaswell merged commit 1b7093d into matplotlib:main May 10, 2024
42 of 44 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request May 10, 2024
@QuLogic
Copy link
Member Author

QuLogic commented May 10, 2024

Err, that was a bit premature as tests weren't passing yet.

@QuLogic QuLogic deleted the old-ipython branch May 10, 2024 18:18
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request May 10, 2024
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).
@tacaswell
Copy link
Member

🤦🏻 Sorry, I only saw the coverage failure and missed the real failure.

QuLogic added a commit to meeseeksmachine/matplotlib that referenced this pull request May 10, 2024
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).
ksunden added a commit that referenced this pull request May 13, 2024
ksunden added a commit that referenced this pull request May 13, 2024
…205-on-v3.9.x

Backport PR #28205 on branch v3.9.x (TST: Fix tests with older versions of ipython)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Qt test_ipython fails on older ipython
3 participants