Skip to content

TST: fix compatibility pytest 8.0 - #15809

Merged
pllim merged 2 commits into
astropy:mainfrom
neutrinoceros:tst/pytest8_compat
Jan 5, 2024
Merged

pllim merged 2 commits into
astropy:mainfrom
neutrinoceros:tst/pytest8_compat

Conversation

@neutrinoceros

Copy link
Copy Markdown
Contributor

Description

This pull request is to address issues seen with pytest 8.0.0rc1, which makes previously hidden warnings visible in tests that use pytest.warns
Fixes #15807

I went with the simplest approach (besides straightforwardly ignoring newly discovered warnings) of blessing each warning as "expected", and just reject anything new that would appear in the future, but maybe in some cases these "new" warnings should be regarded as bugs, in which case it's not just a matter of adjusting tests, but I'll leave that up to be discussed by subpackages maintainers.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

github-actions Bot commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

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 instructions for rebase and squash.
  • 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

github-actions Bot commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

👋 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?

@neutrinoceros
neutrinoceros marked this pull request as ready for review January 4, 2024 11:15
@pllim pllim added this to the v6.0.1 milestone Jan 4, 2024
@pllim

pllim commented Jan 4, 2024

Copy link
Copy Markdown
Member

Thanks! Failures only show up in the predeps job in daily cron, so I just kicked off the cron jobs.

Maybe I need to add remote data directive to the devinfra job below because these were obviously overlooked. Not sure why I didn't catch these in #15054 ; that PR also had cron jobs run.

- name: Python 3.11 with dev version of infrastructure dependencies
os: ubuntu-latest
python: '3.11'
toxenv: py311-test-devinfra

@pllim

pllim commented Jan 4, 2024 •

Copy link
Copy Markdown
Member

Huh why is devinfra even failing here? I have restarted a previously passing build on main from 3 days ago to see if this is even related or not to this PR: https://github.com/astropy/astropy/actions/runs/7375363390

pluggy._manager.PluginValidationError: Plugin 'pytest_filter_subpackage' for hook 'pytest_ignore_collect'
hookimpl definition: pytest_ignore_collect(path, config)

UPDATE: Previously successful build now fails on rerun, so we can ignore devinfra failure in this PR.

@pllim

pllim commented Jan 4, 2024

Copy link
Copy Markdown
Member

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Oh good catch. You actually reported it already but I didn't see it locally so I assumed it was gone. I can look this one first thing tomorrow but feel free to push to this branch in the mean time if it's considered urgent.

@pllim

pllim commented Jan 4, 2024

Copy link
Copy Markdown
Member

if it's considered urgent

Since pytest 8.0 is still in RC stage and not released, not super urgent. I can wait. Thanks!

@neutrinoceros
neutrinoceros requested a review from saimn as a code owner January 5, 2024 23:12

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The checks are probably too detailed but does not hurt. If I were to fix this from scratch, I would probably only focused on the original message and let the rest pass through, to avoid so many extra checks. But given that the work is already done and CI is okay with it, let's keep this and simplify in the future if we have to.

Thanks!

Not sure about the remaining ignore_sigint failure as I also cannot reproduce it locally. Looks like it is used during buffer flushing in hdulist.py. Though who are we to deny people from corrupting their own data if they choose to send a sigint when they flush buffer? 👹

@pllim

pllim commented Jan 5, 2024

Copy link
Copy Markdown
Member

I'll worry about the remaining failures later. One fire at a time. Thanks!

@pllim

pllim commented Jan 5, 2024 •

Copy link
Copy Markdown
Member

Note for my future self: I was thinking about adding --remote-data to devinfra to catch things like this earlier but then I realized devdeps would also pick up pre-releases, and devdeps runs a lot via CI for PRs, so maybe existing setup is enough? If someone feels otherwise, feel free to open follow-up issue or PR.

UPDATE: I think I confused myself. --remote-data isn't the problem here. predeps install all dependencies but not devdeps and not sure if we want to.

pllim added a commit that referenced this pull request Jan 6, 2024
…809-on-v6.0.x

Backport PR #15809 on branch v6.0.x (TST: fix compatibility pytest 8.0)
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.

Test failures in predeps job (pre-release dependencies)

2 participants