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

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

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)

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

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.

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.
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
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.

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!

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.

Upgrade to new-style hook wrappers
3 participants