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

How should we handle PEP-654 ExceptionGroups containing pytest XFailed or Skipped exceptions? #9680

Closed
Zac-HD opened this issue Feb 13, 2022 · 3 comments
Labels
type: enhancement new feature or API change, should be merged into features branch

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Feb 13, 2022

PEP-654 ExceptionGroup has shipped in the 3.11 alphas, and there's an exceptiongroup backport on PyPI. We already have an issue about supporting the tracebacks (#9159) and raising ExceptionGroup from our own code (#8217).

But what happens if an ExceptionGroup is raised from test code, and it contains a special Pytest exception? Worse, what if it contains more than one kind of special exception, i.e. the test is both skipped and xfailed? e.g.

BaseExceptionGroup("msg", [ValueError(), Skipped(), XFailed()])

My proposal is that XFailed should be dropped from exception groups, since it represents a non-error case. Then, if the remainder of the exception group contains a Skipped exception the test is skipped. Otherwise, we treat it as an ordinary failure (with a fancier traceback). Note that for efficiency reasons we might implement these checks in reverse order 😉

(This is a pretty niche edge case to care about, but I just sat down to handle it in Hypothesis and quickly realized that it might depend on how the test runner handles the situation.)

@Zac-HD Zac-HD added the type: enhancement new feature or API change, should be merged into features branch label Feb 13, 2022
@The-Compiler
Copy link
Member

Forgive my ignorance, but I wonder if/why pytest needs to handle those at all? I think it would be good if you could elaborate a bit about Hypothesis' use case there.

While Skipped and XFailed are technically reachable via public API (via pytest.(skip|xfail).Exception, see usage in the wild), they seem to be undocumented - I'm thus inclined to say that pytest is safe to assume that code outside of pytest needs to at least accommodate for its internals. Therefor, it follows that pytest does not need to take care of this as long as it doesn't raise things that way.

@Zac-HD
Copy link
Member Author

Zac-HD commented Feb 13, 2022

Suppose that the user's test might call pytest.skip(), pytest.xfail(), or do something else depending on the input provided by Hypothesis. This would be a very strange test to write directly, but it's not that strange once you start using test helper functions and mixing in pytest.mark.parametrize - I've seen several in the wild, and written+fixed it a few times myself.

Hypothesis has just a touch of special logic for Skipped, in that (like unittest.SkipTest) we immediately reraise it without shrinking. What I need to know is: should I do the same for an ExceptionGroup containing a skip exception? Maybe not, in which case the status quo is fine!

XFailed, from Hypothesis internal's point of view, is just another exception meaning "the test failed", and any interpretation is not Hypothesis' problem.

@Zac-HD
Copy link
Member Author

Zac-HD commented Jun 1, 2022

On considerably more reflection, and investigation how the ecosystem would look across Pytest, Hypothesis, and Unittest, I've concluded that we should not have any handling for ExceptionGroups containing special marker exceptions.

  • Frameworks or libraries like Hypothesis should be (and are) careful to raise marker exceptions directly.
  • If user-supplied test code raises an ExceptionGroup, that's not a Skipped or XFailed exception. It's also much safer to fail loudly when a skip/xfail was expected than vice-versa.
  • It seems infeasible to coordinate the ecosystem on any other exact behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

2 participants