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.

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

MNT: Compat with pytest 8.1 #219

Merged
merged 5 commits into from
Jan 10, 2024
Merged

MNT: Compat with pytest 8.1 #219

merged 5 commits into from
Jan 10, 2024

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jan 6, 2024 •
edited
Loading

To avoid this in pytest 8.1.dev:

pluggy._manager.PluginValidationError: Plugin 'pytest_mpl' for hook 'pytest_report_header'
hookimpl definition: pytest_report_header(config, startdir)
Argument(s) {'startdir'} are declared in the hookimpl but can not be found in the hookspec

See pytest-dev/pytest#11779 (comment)

Fix #216 , xref pytest-dev/pytest#11714 (comment)

@pllim pllim mentioned this pull request Jan 6, 2024
Merged
1 task
bsipocz
bsipocz reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Could you also revert bd7b5c6, so this could be tested?

(and suggestion for the maintainers: add a job for testing with the 8.0.x branch (RC) just as well with pytestdev)

@pllim
Copy link
Contributor Author

pllim commented Jan 9, 2024

Good catch; reverted. Thanks!

@pllim
Copy link
Contributor Author

pllim commented Jan 9, 2024

I have no idea what this test failure is in pytest-dev job

E                   ValueError: I/O operation on closed file

FAILED ../../tests/test_baseline_path.py::test_config[None-None-dir3-dir3-True]

@pllim
Copy link
Contributor Author

pllim commented Jan 9, 2024

That commit I reverted was from #215 by @ConorMacBride . I have no idea what he meant by "Skip pytestdev until hook wrapper issue is fixed" as there is no further explanation in the PR nor any linked issue.

@ConorMacBride
Copy link
Member

The hook wrapper issue is #216

ConorMacBride
ConorMacBride reviewed Jan 9, 2024
View reviewed changes
Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

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

When I tested a fix for #216 last month, the tests passed with the pytest main branch installed. I did not see any mention of this issue, which seems strange as I would have expected the deprecation warning to be raised as an exception. Have you seen this pytest-mpl hook raise an exception?

pytest_mpl/plugin.py Show resolved Hide resolved
@bsipocz
Copy link
Contributor

bsipocz commented Jan 9, 2024

I would have expected the deprecation warning to be raised as an exception

We didn't see deprecation warnings in other plugins either and similar failures popped up when they became actual errors. I haven't dug deep enough to get a full understanding but the plugins/pytest doesn't always behave the same predictable way as a normal test dependency library.

@pllim
Remove args from pytest_report_header
e57d11e 
and fix up the other pytest 8 compat problem with old-style hook wrapper.
Update flake8 setting to reflect what is actually in the code.
@pllim
Auto-cancel duplicate CI
32da10a 
and remove unnecessary pytest version check I added earlier.
@pllim
Copy link
Contributor Author

pllim commented Jan 9, 2024

pytestdev is green but the log has traceback, is this expected? I don't maintain this plugin so I don't know what the test is supposed to do.

https://github.com/matplotlib/pytest-mpl/actions/runs/7464235520/job/20310772695?pr=219

@pllim
Copy link
Contributor Author

pllim commented Jan 9, 2024

Would be nice to get this merged sooner than later if this patch is acceptable. astropy devinfra job is failing without this.

@pllim pllim mentioned this pull request Jan 9, 2024
Merged
1 task
@pllim
Copy link
Contributor Author

pllim commented Jan 10, 2024

@ConorMacBride , does this look good to you now? Can we merge this? I don't have write access to this repo but astropy depends on it.

ConorMacBride
ConorMacBride approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

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

Thanks @pllim and @bsipocz!

@ConorMacBride ConorMacBride merged commit db0ea36 into matplotlib:main Jan 10, 2024
21 checks passed
@pllim pllim deleted the patch-1 branch January 10, 2024 21:11
@pllim pllim mentioned this pull request Jan 24, 2024
Closed
@ConorMacBride ConorMacBride mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsipocz bsipocz bsipocz left review comments

@ConorMacBride ConorMacBride ConorMacBride approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to new-style hook wrappers
3 participants
@pllim @ConorMacBride @bsipocz