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

Add support for classes with pytest 7 #164

Merged
merged 5 commits into from Jun 14, 2022

Conversation

ConorMacBride
Copy link
Member

@ConorMacBride ConorMacBride commented Jun 1, 2022

Since pytest 7 was released pytest-mpl has not been running tests inside classes. This means that figure tests inside classes are always being reported as passing. In this PR, I have added additional tests that ensure figure tests inside classes can fail correctly, and I have also added new testing jobs for older versions of pytest as well as the development version.

  • Find the bug and fix it
  • Make a bugfix release of pytest-mpl

Solution

Instead modifying the test function itself by wrapping it in a function which runs the tests, this PR uses pytest hooks to intercept the generated figure and then run the tests. This should be a more robust approach that doesn't need as many special cases to be hardcoded (such as classes and support for setup_method etc.).

Previously we updated the test function (to a version of itself but wrapped in the testing code) inside the pytest_runtest_setup hook. However, now we work inside the pytest_runtest_call hook and override pytest's default pytest_pyfunc_call hook:

  1. A test starts
  2. Pytest calls our pytest_runtest_call hook
  3. It runs until yield
  4. Pytest then calls our pytest_pyfunc_call hook which runs the test function, intercepts the returned figure, and saves it as an attribute (self.result)
  5. Pytest then continues after the yield inside our pytest_runtest_call hook
  6. It accesses the figure from the attribute self.result
  7. It tests the figure

I have also combined the get_marker and get_compare functions/methods to simplify these.

@lgtm-com
Copy link

lgtm-com bot commented Jun 2, 2022

This pull request introduces 2 alerts when merging 83359e6 into b8bcfe1 - view on LGTM.com

new alerts:

  • 2 for Unused import

Instead modifying the test function itself by wrapping it in a function which runs the tests, use pytest hooks to intercept the generated figure and then run the tests. This should be a more robust approach that doesn't need as many special cases to be hardcoded.

I have also refactored the get_marker and get_compare functions/methods to simplify these.
@ConorMacBride
Copy link
Member Author

To ensure pytest-mpl works across different versions of pytest, I have added some new tests:

- linux: py310-test-mpl35-pytestdev
- linux: py310-test-mpl35-pytest62
- linux: py38-test-mpl35-pytest54

Since pytest doesn't seem to release bug fixes for previous major/minor versions, I have just added tests for the latest release for each major version from the past ~2 years. (We should probably set up a weekly cron for the tests also?)

I've also added a test using the development version of pytest. (This uncovered that pytest are going to start warning and eventually erroring if a test returns anything other than None, however provided pytest-mpl is installed, this warning will not be shown.)

@ConorMacBride ConorMacBride marked this pull request as ready for review June 2, 2022 16:25
Comment on lines -218 to -220
# "item.keywords.get" was deprecated in pytest 3.6
# See https://docs.pytest.org/en/latest/mark.html#updating-code
return item.keywords.get(marker_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need to support this branch anymore.

@ConorMacBride
Copy link
Member Author

I've also updated the generate_test_name method to include the class name in the test name (if test is inside a class): 19dac01

This is technically a bugfix as the following should be able to generate three different baselines, but currently without the class name they overwrite each other and only the final test will pass when tested:

class TestA:
    @pytest.mark.mpl_image_compare
    def test_a(self): ...
class TestB:
    @pytest.mark.mpl_image_compare
    def test_a(self): ...
@pytest.mark.mpl_image_compare
def test_a(self): ...

This is a breaking change for figure tests within classes only, and the following will need to be done:

  • Hash library test names will need to be regenerated/updated to include the class name.
  • Iff the undocumented mpl-use-full-test-name ini option is enabled, the the baseline images will need to be regenerated, or have their filename updated to include the class name.

Thought I would include this here while we are fixing the classes.

@ConorMacBride
Copy link
Member Author

@Cadair I ran the astropy and sunpy figure tests against this branch and pytest-mpl worked correctly, with most tests passing and some failing because of the different mpl version I used. Both produced a complete HTML report without any tests missing.

@Cadair Cadair added the bug label Jun 14, 2022
Copy link
Contributor

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

The tests pass so it's probably fine 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants