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

tests: don't print xfails #1865

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

henryiii
Copy link
Contributor

These xfails are being printed as tracebacks on every test run. Since they are known to not work, let's hide them for now.

@webknjaz
Copy link
Member

I think this is an incorrect use of xfail(run=False). Doing this would make their existence in the codebase pointless. The tracebacks are only shown in pytest 8, not older — perhaps, there's a report setting to simply hide them.

The idea is that the xfails may start reporting XPASS at some point if fixed accidentally.

Here's some of @pganssle's explanations on doing it properly: https://blog.ganssle.io/articles/2021/11/pytest-xfail.html / https://ganssle.io/talks/#xfail-and-skip / https://ganssle.io/talks/#xfail-lightning.

@henryiii
Copy link
Contributor Author

This was broken by pytest-dev/pytest#11233. And it was reported upstream in pytest-dev/pytest#12231, and fixed in pytest-dev/pytest#12280.

So we can either wait till a pytest release contains this fix, or temporarily turn them off.

I think this is an incorrect use of xfail(run=False)

That's why this change was bad; xfails are expected to fail, so printing an exception traceback explaining the known failure pollutes the logs and pretty much forces these to be turned off. It will is listed in the summary at the end (which is what the -rx part of -ra is for), so someone is still gently reminded (with three lines instead of hundreds) that these xfail tests exist and can be worked on. (The new version will have an optional flag to see the traceback, which is far better).

@webknjaz
Copy link
Member

@henryiii yes, so I'd suggest setting -rfEsX instead (excluding x there but including X). You'd correct the output which is what's annoying you but would still get the benefits of being alerted about XPASS (strict). On top of that, you could add some linting check for when the pytest version is higher than 8.2.2 there shouldn't be a non -ra reporting setting in the config. You could stick this into a conftest.py or an arbitrary *_test.py.

@henryiii henryiii force-pushed the henryiii/tests/cleanup branch from c36a32a to bdfb0bd Compare June 11, 2024 20:16
@henryiii
Copy link
Contributor Author

I just added a todo, don't think we need anything too fancy, and we don't have to drop it the second pytest releases. ;) Otherwise followed your suggestion and put that sequence of letters in.

@henryiii henryiii merged commit 8d86d31 into pypa:main Jun 12, 2024
21 of 23 checks passed
@henryiii henryiii deleted the henryiii/tests/cleanup branch June 12, 2024 14:46
m-richards added a commit to m-richards/cibuildwheel that referenced this pull request Jul 22, 2024
henryiii pushed a commit that referenced this pull request Jul 23, 2024
Revert "tests: don't print xfails (#1865)"

This reverts commit 8d86d31.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants