Skip to content

pytest 8 compatibility#15054

Merged
pllim merged 3 commits intoastropy:mainfrom
pllim:pytest-8-compat
Aug 17, 2023
Merged

pytest 8 compatibility#15054
pllim merged 3 commits intoastropy:mainfrom
pllim:pytest-8-compat

Conversation

@pllim
Copy link
Copy Markdown
Member

@pllim pllim commented Jul 10, 2023 •
edited
Loading

Description

This pull request is to address pytest 8 compatibility, which also allows us to fix some poor test warnings handling. Also see pytest-dev/pytest#10937 (comment)

Fixes #15015

  • Finish fixing stuff, ran out of time...
  • In a separate PR, pin maxversion of pytest in v5.0.x because I don't want to redo this in old LTS branch. See MNT: Pin pytest<8 for v5.0.x #15060

After merge: Open follow-up issue to just require pytest>=8 in the future so we can clean up the if-else logic here.

@pllim pllim added this to the v5.3.1 milestone Jul 10, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 10, 2023

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • 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 we cannot check for it on Actions; 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 10, 2023

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim

This comment was marked as resolved.

Sign in to view
@pllim pllim force-pushed the pytest-8-compat branch from 3cf451b to 3c8dbcf Compare July 11, 2023 01:43
@pllim pllim added the Extra CI Run cron CI as part of PR label Jul 11, 2023
pllim
pllim commented Jul 11, 2023
View reviewed changes
pllim
pllim commented Jul 11, 2023
View reviewed changes
@pllim
Copy link
Copy Markdown
Member Author

pllim commented Jul 12, 2023

pre-commit.ci autofix

@pllim pllim force-pushed the pytest-8-compat branch from ecaf22a to 8796982 Compare July 12, 2023 02:08
@pllim pllim mentioned this pull request Jul 12, 2023
Merged
@pllim pllim marked this pull request as ready for review July 12, 2023 06:02
@pllim pllim requested review from a team, Cadair, astrofrog, mcara, saimn and taldcroft as code owners July 12, 2023 06:02
@pllim pllim requested a review from nstarman July 12, 2023 06:02
@pllim pllim modified the milestones: v5.3.2, v5.3.3 Aug 15, 2023
pllim added 2 commits August 15, 2023 17:25
@pllim
MNT: pytest 8 compatibility
53ba76a 
[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
@pllim
More descriptive ctx names
b3e70b9 
as requested by nstarman
@pllim pllim force-pushed the pytest-8-compat branch 3 times, most recently from 512d14b to fabbc49 Compare August 15, 2023 22:12
pllim
pllim commented Aug 15, 2023
View reviewed changes
@pllim pllim mentioned this pull request Aug 15, 2023
Closed
@pllim
Copy link
Copy Markdown
Member Author

pllim commented Aug 15, 2023

pre-commit.ci autofix

pllim
pllim commented Aug 15, 2023
View reviewed changes
@pllim
Copy link
Copy Markdown
Member Author

pllim commented Aug 16, 2023

Linkcheck failure is unrelated.

@pllim
Simplify io.fits test updates.
5a0d52a 
Do not use raw string for regex when we do not have to.
@pllim
Copy link
Copy Markdown
Member Author

pllim commented Aug 16, 2023

If I don't hear back from @saimn or @astrofrog by 2023-08-17, I am just going to merge this so I can move on with my life.

@pllim pllim merged commit e8b7938 into astropy:main Aug 17, 2023
@lumberbot-app

This comment was marked as resolved.

Sign in to view
@pllim
Copy link
Copy Markdown
Member Author

pllim commented Aug 17, 2023 •
edited
Loading

Ugh, maybe we'll just pin pytest<8 in v5.3.x if pytest 8 is released before we kill v5.3.x series.

@pllim pllim deleted the pytest-8-compat branch August 17, 2023 14:41
@pllim pllim modified the milestones: v5.3.3, v6.0 Aug 17, 2023
@pllim pllim mentioned this pull request Aug 17, 2023
Merged
1 task
saimn
saimn reviewed Aug 29, 2023
View reviewed changes
astropy/io/fits/tests/test_core.py

pytest.raises(
fits.VerifyError, hdul.writeto, filename, output_verify="exception"
)
Copy link
Copy Markdown
Contributor

@saimn saimn Aug 29, 2023

Choose a reason for hiding this comment

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

@pllim - Hmm seems this one got lost while updating the other lines ?

Copy link
Copy Markdown
Member Author

@pllim pllim Aug 29, 2023

Choose a reason for hiding this comment

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

This line is missing a with. Not sure what it is supposed to do.

Copy link
Copy Markdown
Contributor

@saimn saimn Aug 29, 2023

Choose a reason for hiding this comment

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

it can be used as a function :

When using [pytest.raises()](https://docs.pytest.org/en/latest/reference/reference.html#pytest.raises) as a function, you can use: pytest.raises(Exc, func, match="passed on").match("my pattern").)

Copy link
Copy Markdown
Contributor

@saimn saimn Aug 29, 2023

Choose a reason for hiding this comment

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

(which I agree is not usual and obvious ;))

Copy link
Copy Markdown
Member Author

@pllim pllim Aug 29, 2023

Choose a reason for hiding this comment

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

Oh, oops. Do you want to add it back? If not, I'll get to it when I have time. Sorry!

Copy link
Copy Markdown
Contributor

@saimn saimn Aug 29, 2023

Choose a reason for hiding this comment

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

Not sure if it's tested elsewhere, so I will try to keep that on my list. But no problem, it's easy to get confused by this.

Copy link
Copy Markdown
Member Author

@pllim pllim Aug 30, 2023

Choose a reason for hiding this comment

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

See #15252

pllim added a commit to pllim/astropy that referenced this pull request Aug 30, 2023
@pllim
TST: Add back test removed in astropy#15054
61e3bd3 
for io.fits
@pllim pllim mentioned this pull request Aug 30, 2023
Merged
1 task
@pllim pllim mentioned this pull request Jan 4, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn left review comments

@eerovaher eerovaher eerovaher left review comments

@taldcroft taldcroft taldcroft approved these changes

@nstarman nstarman nstarman approved these changes

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

@Cadair Cadair Awaiting requested review from Cadair

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

Assignees

No one assigned

Labels

Extra CI Run cron CI as part of PR io.fits io.votable modeling nddata table testing utils wcs.wcsapi wcs

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

TST: Expected warnings changed in some tests in devinfra job (41 failures)

6 participants

@pllim @nstarman @saimn @astrofrog @taldcroft @eerovaher