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

Fix tests which exit before returning a figure or use unittest.TestCase #171

Merged
merged 7 commits into from Jul 22, 2022

Conversation

ConorMacBride
Copy link
Member

Fixes #170

Currently pytest-mpl relies on the pytest_pyfunc_call hook to intercept the figure for testing from the test function. However, when using unittest.TestCase this hook is not used. Rather than adding a specific fix for unittest.TestCase, in this PR I have refactored pytest-mpl such that, instead of using the pytest_pyfunc_call hook, within the pytest_runtest_call hook pytest-mpl will,

  1. wrap the user's test function in a wrapper which stores the returned figure in the plugin, and makes the user's test function return None instead, (pytest will require None to be returned in a future version.)
  2. yield to pytest so pytest can call the test function (either with its pytest_pyfunc_call, or otherwise in the case of unittest.TestCase),
  3. retrieve the returned figure from the plugin object (i.e. self),
  4. carry on as usual with the testing of the returned figure.

I've added tests to ensure pytest-mpl can both pass and fail as expected when testing using unittest.TestCase.

Just to be sure pytest-mpl now works with a variety of different testing setups, before we make a bugfix release I want to add tests to tests/subtests for testing classes. I'll do that in a separate PR.

Rather than overriding the low level `pytest_pyfunc_call`, we can wrap the test function in a wrapper that stores its return value to the plugin object.
If the return value hasn't been added to the dictionary, it means that the user's test didn't reach the end. This means that it raised an exception or was otherwise stopped early by pytest (skipped, failed, etc). In any case, we should not proceed with figure testing and use the result that pytest has already determined.

Fixes matplotlib#172
@ConorMacBride
Copy link
Member Author

I've added tests in 32b9a12 which reproduce the bug (and various similar bugs) reported in #172. You can see that these tests fail as expected: https://github.com/matplotlib/pytest-mpl/actions/runs/2583532536

I've added a fix for this bug in d84892b, and you can see that the new tests now pass: https://github.com/matplotlib/pytest-mpl/actions/runs/2583592229

In this PR the user's test function is now wrapped in a wrapper that adds the figure to a dictionary with a key which is the test name. If for whatever reason the user's test function exits early, nothing will be added to the dictionary. Therefore, if a key with the current test name doesn't exist in the dictionary, we skip image comparison testing and let pytest handle the error/skip etc. as usual.

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 seems reasonable to me, but probably complicated enough to have another set of eyes.

@ConorMacBride ConorMacBride changed the title Fix tests which use unittest.TestCase Fix tests which exit before returning a figure or use unittest.TestCase Jul 22, 2022
@ConorMacBride
Copy link
Member Author

Got 2nd approval from @Cadair in a message.

@ConorMacBride ConorMacBride merged commit 310ee99 into matplotlib:main Jul 22, 2022
dpanici added a commit to PlasmaControl/DESC that referenced this pull request Sep 8, 2022
Documents kwargs for plotting functions and adds sensible kwargs to those that were lacking them (such as figsize)
 - Document kwargs, according to [matplotlib-style documentation](https://stackoverflow.com/questions/62511086/how-to-document-kwargs-according-to-numpy-style-docstring)
 - add kwargs to plotting functions missing sensible/useful kwargs
 - add check for unused kwargs to most plotting functions
 - add plot comparisons to artifact uploaded to github 
 - bump `pytest-mpl` version to fix [bug](matplotlib/pytest-mpl#171) where if `unittest.TestCase` is used, it did not actually run those tests correctly 

resolves #178 , #244
@ConorMacBride ConorMacBride deleted the fix-unittest branch February 28, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants