MNT: Compat with pytest 8.1 by pllim · Pull Request #219 · matplotlib/pytest-mpl · GitHub
Skip to content

MNT: Compat with pytest 8.1#219

Merged
ConorMacBride merged 5 commits into
matplotlib:mainfrom
pllim:patch-1
Jan 10, 2024
Merged

MNT: Compat with pytest 8.1#219
ConorMacBride merged 5 commits into
matplotlib:mainfrom
pllim:patch-1

Conversation

@pllim

@pllim pllim commented Jan 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Contributor

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

pllim commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

@pllim

pllim commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

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

pllim commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

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

ConorMacBride commented Jan 9, 2024

Copy link
Copy Markdown
Member

The hook wrapper issue is #216

ConorMacBride
ConorMacBride reviewed Jan 9, 2024
View reviewed changes

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

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?

Comment thread pytest_mpl/plugin.py
@bsipocz

bsipocz commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

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

pllim commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

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

pllim commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

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

pllim commented Jan 10, 2024

Copy link
Copy Markdown
Contributor Author

ConorMacBride
ConorMacBride approved these changes Jan 10, 2024
View reviewed changes

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

Thanks @pllim and @bsipocz!

@ConorMacBride ConorMacBride merged commit db0ea36 into matplotlib:main Jan 10, 2024
@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

@ConorMacBride ConorMacBride ConorMacBride approved these changes
+1 more reviewer
@bsipocz bsipocz bsipocz left review comments
Reviewers whose approvals may not affect merge requirements

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