Skip to content

Fix testing for pytest 7.2 support #13892

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.

Sign up for GitHub

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

Jump to bottom
Merged
pllim merged 2 commits into from
Oct 25, 2022
Merged

Fix testing for pytest 7.2 support #13892

pllim merged 2 commits into from
Oct 25, 2022

Conversation

@WilliamJamieson
Copy link
Contributor

@WilliamJamieson WilliamJamieson commented Oct 25, 2022 •
edited by pllim
Loading

Description

pytest 7.2 was just released. It deprecates several things that astropy was using for testing, including setup and teardown functions. This PR fixes all those deprecations by switching them over to setup_method and teardown_method.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@WilliamJamieson WilliamJamieson requested a review from pllim October 25, 2022 14:36
@WilliamJamieson WilliamJamieson requested review from a team, mcara, saimn and taldcroft as code owners October 25, 2022 14:36
@WilliamJamieson WilliamJamieson mentioned this pull request Oct 25, 2022
Closed
@WilliamJamieson WilliamJamieson added this to the v5.0.6 milestone Oct 25, 2022
@WilliamJamieson WilliamJamieson mentioned this pull request Oct 25, 2022
Merged
10 tasks
pllim
pllim reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Something else needs updating to get past the CI failure of this PR.

Thanks for tackling this!

astropy/coordinates/tests/test_celestial_transformations.py
"""

def setup(self):
def setup_method(self):
Copy link
Member

@pllim pllim Oct 25, 2022

Choose a reason for hiding this comment

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

Why not setup_class? Does this need to be redefined before every method test?

Same question for all other similar changes.

Copy link
Contributor Author

@WilliamJamieson WilliamJamieson Oct 25, 2022

Choose a reason for hiding this comment

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

According to https://docs.pytest.org/en/7.2.x/deprecations.html#nose-deprecation setup_method teardown_method are the replacements for setup and teardown.

pllim
pllim reviewed Oct 25, 2022
View reviewed changes
astropy/visualization/wcsaxes/tests/test_frame.py
# Clip the image to the frame
im.set_clip_path(ax.coords.frame.patch)

return fig
Copy link
Member

@pllim pllim Oct 25, 2022

Choose a reason for hiding this comment

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

Are you sure this isn't needed for them weird hash image comparison that CircleCI picks up?

Copy link
Member

@pllim pllim Oct 25, 2022

Choose a reason for hiding this comment

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

I think this is intentional... #5496 cc @astrofrog

Copy link
Contributor Author

@WilliamJamieson WilliamJamieson Oct 25, 2022

Choose a reason for hiding this comment

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

Per the pytest changelog:

#10196:` PytestReturnNotNoneWarning is now a subclass of PytestRemovedIn8Warning: the plan is to make returning non-None from tests an error in the future.

Copy link
Member

@pllim pllim Oct 25, 2022

Choose a reason for hiding this comment

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

Looks like pytest isn't gonna back down, so we're blocked by matplotlib/pytest-mpl#183 . I see two immediate actions we can take:

  • Pin maxversion of pytest to <7.2 to immediately unblock all the other PRs for feature freeze week. Turn this PR to draft until upstream can figure out the compatibility.
  • Continue with this PR but use pytest.mark.filterwarnings to ignore this warning for the affected image tests. But this is also risky because if we forget and they turn warning into exception in a future release, we still have to pin maxversion then.

What to do?

This was referenced Oct 25, 2022
Merged
Closed
Closed
@WilliamJamieson WilliamJamieson force-pushed the bugfix/pytest branch 2 times, most recently from a482a36 to 0ed693d Compare October 25, 2022 18:28
pllim
pllim reviewed Oct 25, 2022
View reviewed changes
pllim
pllim approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

The diff looks reasonable to me and skipping is fine, since we don't calculate coverage for remote data anyway. Thanks!

@pllim pllim merged commit fb3b2ad into astropy:main Oct 25, 2022
@lumberbot-app

This comment was marked as resolved.

Sign in to view
@WilliamJamieson WilliamJamieson deleted the bugfix/pytest branch October 25, 2022 19:40
WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Oct 25, 2022
@WilliamJamieson
Merge pull request astropy#13892 from WilliamJamieson/bugfix/pytest
0c589c6 
Fix testing for `pytest` 7.2 support
@WilliamJamieson WilliamJamieson mentioned this pull request Oct 25, 2022
Merged
10 tasks
pllim added a commit that referenced this pull request Oct 25, 2022
@pllim
Merge pull request #13895 from WilliamJamieson/backport/13892
d72ab6b 
Manual backport of #13892
@WilliamJamieson WilliamJamieson mentioned this pull request Oct 26, 2022
Closed
pllim
pllim reviewed Nov 15, 2022
View reviewed changes
astropy/tests/figures/helpers.py
style=style,
savefig_kwargs=savefig_kwargs,
**kwargs)
@pytest.mark.skipif(not HAS_PYTEST_MPL, reason='pytest-mpl is required for the figure tests')
Copy link
Member

@pllim pllim Nov 15, 2022

Choose a reason for hiding this comment

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

I thought about reverting this and use -m "not mpl_image_compare" as suggested in matplotlib/pytest-mpl#183 (comment) but I cannot figure out a sane way to define that in tox.ini because we have a few possible flag combos already and CircleCI can theoretically use any of those flags if it wants to.

astropy/tox.ini

Lines 119 to 124 in 7e80b9f

commands =
pip freeze
!cov-!double: pytest --pyargs astropy {toxinidir}/docs {env:MPLFLAGS} {posargs}
cov-!double: pytest --pyargs astropy {toxinidir}/docs {env:MPLFLAGS} --cov astropy --cov-config={toxinidir}/pyproject.toml {posargs}
double: python -c 'import sys; from astropy import test; test(); sys.exit(test())'
cov: coverage xml -o {toxinidir}/coverage.xml

Maybe this is still the best option we have unless people have better ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes

@mcara mcara Awaiting requested review from mcara mcara is a code owner

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft is a code owner

@saimn saimn Awaiting requested review from saimn saimn is a code owner

@astrofrog astrofrog Awaiting requested review from astrofrog

@Cadair Cadair Awaiting requested review from Cadair

Assignees

No one assigned

Labels

coordinates io.fits modeling no-changelog-entry-needed time units utils.iers wcs

Projects

None yet

Milestone

v5.0.6

Development

Successfully merging this pull request may close these issues.

2 participants

@WilliamJamieson @pllim