Skip to content
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

Adding an autouse fixture to close plots after each test #1628

Merged
merged 1 commit into from Aug 20, 2020

Conversation

greglucas
Copy link
Contributor

Automatically closing figures after each test. Implemented the suggestion from #1625 (comment)

Rationale

Will automatically close figures after each test to not reuse the same figures.

Implications

@greglucas greglucas requested a review from QuLogic August 5, 2020 13:30
@QuLogic
Copy link
Member

QuLogic commented Aug 6, 2020

Can we have a conftest.py also in the mpl directory? We may also want to put a bit of Matplotlib testing reset into the beginning of this function too.

@greglucas
Copy link
Contributor Author

Good suggestion, I just moved everything over to the mpl-specific directory so that it only gets called there and not in every test.

What other testing reset do you think we need? Do you want to run each test in an rccontext so that any rcparams don't get propagated as well?

@greglucas
Copy link
Contributor Author

Just realized the actual MPL fixture is not private. I was looking at _cleanup_cm this whole time 🤦‍♂️ I just pushed a new version to bring that directly into the new conftest file.

@dopplershift
Copy link
Contributor

I'm not sure how I feel about this:

  1. Importing from matplotlib's conftest feels weird, I wouldn't necessarily consider that public API
  2. mpl_test_settings is all of matplotlib's test settings, beyond using the figure auto-closer.

@QuLogic ?

@greglucas
Copy link
Contributor Author

greglucas commented Aug 7, 2020

That is one of the downsides of force-pushing over things with multiple implementations. Sorry about that, Ryan. The previous iteration I had in here was simply:

@pytest.fixture(autouse=True)
def close_plots(request):
    try:
        yield
    finally:
        plt.close("all")

Actually, it looks like using MPLs version doesn't work on Python 3.5 for us either, so maybe the previous iteration is the better route anyways.

@greglucas
Copy link
Contributor Author

I just undid the "simple" version of grabbing MPLs test fixture.

I put the yield into a context manager. Of course, on my macosx backend that broke some tests :( The fix for me locally was to force all mpl tests to use the Agg backend by default (The image testing decorator switches to TkAgg when necessary for show() calls), is this an OK approach?

This takes care of all the cleanup of figures and state
for each test.
Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me. It's straightforward. The Agg switching is something I've done on MetPy as well (in a different kind of global fixture.)

yield
finally:
# Closes all open figures and switches backend back to original
plt.switch_backend(orig_backend)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea switch_backend closed all the figures, but that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me either until I went through MPLs cleanup decorator. Everyones learning something today :)

try:
# Run each test in a context manager so that state does not leak out
orig_backend = plt.get_backend()
plt.switch_backend('Agg')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@greglucas greglucas added this to the 0.19 milestone Aug 20, 2020
@QuLogic QuLogic merged commit 509798b into SciTools:master Aug 20, 2020
@greglucas greglucas deleted the close_test_figures branch August 20, 2020 03:56
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.

None yet

3 participants