first draft of handling marker exceptions wrapped in exceptiongroup

[jakkdl]

I have barely used Hypothesis, and never worked with the codebase, so there are several gaps in code coverage and question marks on implementation decisions. If somebody more familiar wants to take a quick pass and opine it'd be very helpful :)

I can get code coverage by manually raising the specific ExceptionGroups that'll trigger the different branches, but it'd likely be better to do the actual things that causes those exceptions. So would likewise appreciate quick pointers on how to, if at all possible, cause e.g. multiple StopTest, multiple Frozen, a single Frozen, etc.

fixes #4106

[jobh]

Very nice, thanks! 👍

I think most of the build errors will go away with some of the minor commented changes, the remaining will hopefully be manageable :-)

[jobh]

Yep, the semantics here (generally) are a good question.

I'd opine:

• If all exceptions are Frozen, raise the first (as above).
• Else, if exactly one exception is non-Frozen, raise that one [*].
• Otherwise, reconstruct and raise a group with just the non-frozen ones (maybe .with_traceback(eg.__traceback__)).

[*] hm, I see, that would cause an undesired unwrapping if user code raises a single-exception ExceptionGroup. Maybe the second step should be "if exactly one ... and that exception is a StopException or HypothesisException.

[jakkdl]

If all exceptions are Frozen, raise the first (as above).

Actually, I think this could cause confusion. If I spawn three tasks and I only get one Frozen, I will assume that the-thing-that-caused-Frozen only happened in one of the tasks. We should probably err on the side of not unwrapping unless necessary to get internal exception handling to work.

edit: nvm, I thought frozen didn't get swallowed by @given

edit2: I'm confused about how hypothesis works 😅

edit3: Aaaahhh.

hypothesis/hypothesis-python/src/hypothesis/core.py

Lines 1077 to 1089 in 4d079f7

	except failure_exceptions_to_catch() as e:
	# If an unhandled (i.e., non-Hypothesis) error was raised by
	# Hypothesis-internal code, re-raise it as a fatal error instead
	# of treating it as a test failure.
	filepath = traceback.extract_tb(e.__traceback__)[-1][0]
	if is_hypothesis_file(filepath) and not isinstance(e, HypothesisException):
	raise
	if data.frozen:
	# This can happen if an error occurred in a finally
	# block somewhere, suppressing our original StopTest.
	# We raise a new one here to resume normal operation.
	raise StopTest(data.testcounter) from e

This means that if we got our data frozen, any exceptions (no matter if inside a group or not) get disregarded. So it doesn't matter what we do if there's only frozen exceptions

edit4: and this is very bad, because it can suppress TypeError and anything. Gotta rejig some logic here

edit5: except that's what the code currently does in sync code, so... ?

[jakkdl]

Another question is if we get multiple HypothesisException/StopException, if that's possible?

[Zac-HD]

If we get multiple StopTest, re-raise the one with the lowest _testcounter i.e. the first one to occur.

[jobh]

I'd just catch BaseExceptionGroup (via hypothesis.internal.compat.BaseExceptionGroup) and check the subexceptions explicitly, to avoid the trio dependency.

[jakkdl]

ye that's probably the way to go. I'm incredibly biased on the matter as the person who wrote trio.testing.RaisesGroup and is trying to get it added to pytest 😇

[Zac-HD]

(and as a dual maintainer of Pytest and Trio, among other things, I'm pretty keen to support that when I have time 😅)

[jobh]

I can get code coverage by manually raising the specific ExceptionGroups that'll trigger the different branches, but it'd likely be better to do the actual things that causes those exceptions. So would likewise appreciate quick pointers on how to, if at all possible, cause e.g. multiple StopTest, multiple Frozen, a single Frozen, etc.

It should be possible to provoke most of these situations by playing around with multiple tasks failing in different ways. But I think it's fine to create the non-obvious situations explicitly, too.

[jobh]

It should be possible to provoke most of these situations by playing around with multiple tasks failing in different ways. But I think it's fine to create the non-obvious situations explicitly, too.

Sorry: "most" should probably be replaced by "some" 😁

[jobh]

Oh, and one other tip: hypothesis will transform/trim the tracebacks by default, which may be confusing sometimes when working on exceptions and not seeing what you'd expect. See get_trimmed_traceback().

[jakkdl]

I can get code coverage by manually raising the specific ExceptionGroups that'll trigger the different branches, but it'd likely be better to do the actual things that causes those exceptions. So would likewise appreciate quick pointers on how to, if at all possible, cause e.g. multiple StopTest, multiple Frozen, a single Frozen, etc.

It should be possible to provoke most of these situations by playing around with multiple tasks failing in different ways. But I think it's fine to create the non-obvious situations explicitly, too.

actually, make that none of the situations if we 1. Want tests to run on py<3.11 and 2. not add a dependency on trio or anyio.
asyncio.TaskGroup wasn't added until py3.11, and afaik nothing in the stdlib will raise ExceptionGroup before that.

[Zac-HD]

I think for testing we'll want /cover/ tests which do whatever explicit super-weird code we need (we have ExceptionGroup), and also some /nocover/ tests which only run on py311+ and use asyncio.TaskGroup in a somewhat-more-natural way. Then when 310 goes end of life, we can promote them over to replace the weird cover tests.

[Zac-HD]

(partial review is better than no review, right?)

I've also merged a fix for #4115 so you might want to rebase on master 🙂

[Zac-HD]

Alas, raise Whatever() from None would defeat this inspection.

[jakkdl]

that would only override the __cause__ no? And the comment specifically talks about errors happening in finally, where you rarely raise explicit exceptions.
It seems pretty dangerous to ignore all exceptions in case the data is frozen, shouldn't the user be informed if there's errors happening in their teardown code?
Although thinking about it more, the problem is perhaps that the StopTest.__cause__ is not getting reraised/logged?

[Zac-HD]

Because Hypothesis does a lot of runtime introspection, we prefer to avoid from __future__ import annotations - which will eventually go away anyway, c.f. PEPs 649 and 749.

[jakkdl]

do you also want to avoid it in test files?

[jakkdl]

#4115 made me realize that unwrapping exceptions inside groups is really tricky to do right without losing the context/cause of the original exception. Though I'm unsure if/how much this matters for hypothesis, as we only try to unwrap specific marker exceptions which users wouldn't see anyway.