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

first draft of handling marker exceptions wrapped in exceptiongroup #4110

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 44 additions & 2 deletions hypothesis-python/src/hypothesis/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
FlakyFailure,
FlakyReplay,
Found,
Frozen,
HypothesisException,
HypothesisWarning,
InvalidArgument,
Expand Down Expand Up @@ -148,6 +149,8 @@
else: # pragma: no cover
EllipsisType = type(Ellipsis)

if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroup
jakkdl marked this conversation as resolved.
Show resolved Hide resolved

TestFunc = TypeVar("TestFunc", bound=Callable)

Expand Down Expand Up @@ -768,6 +771,45 @@ def execute(data, function):
return default_executor


@contextlib.contextmanager
def unwrap_exception_group():
def _flatten_group(excgroup: BaseExceptionGroup) -> list[BaseException]:
found_exceptions = []
for exc in excgroup.exceptions:
if isinstance(exc, BaseExceptionGroup):
found_exceptions.extend(_flatten_group(exc))
else:
found_exceptions.append(exc)
return found_exceptions

try:
yield
except BaseExceptionGroup as excgroup:
# Found? RewindRecursive? FlakyReplay?
marker_exceptions = (StopTest, UnsatisfiedAssumption)
frozen_exceptions, non_frozen_exceptions = excgroup.split(Frozen)
marker_exceptions, user_exceptions = non_frozen_exceptions.split(lambda e: isinstance(e, marker_exceptions))

# TODO: not a great variable name
if user_exceptions:
raise

if non_frozen_exceptions:
flattened_non_frozen_exceptions = _flatten_group(non_frozen_exceptions)
if len(flattened_non_frozen_exceptions) == 1:
raise flattened_non_frozen_exceptions[0] from None
# multiple non-frozen exceptions, re-raise the entire group
raise

flattened_frozen_exceptions = _flatten_group(frozen_exceptions)
# we only have frozen exceptions
if len(flattened_frozen_exceptions) == 1:
raise flattened_frozen_exceptions[0] from excgroup

# multiple frozen exceptions
# TODO: raise a group? The first one? None of them?
raise frozen_exceptions[0] from excgroup

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jakkdl jakkdl Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

class StateForActualGivenExecution:
def __init__(self, stuff, test, settings, random, wrapped_test):
self.test_runner = get_executor(stuff.selfy)
Expand Down Expand Up @@ -841,7 +883,7 @@ def execute_once(

@proxies(self.test)
def test(*args, **kwargs):
with ensure_free_stackframes():
with unwrap_exception_group(), ensure_free_stackframes():
return self.test(*args, **kwargs)

else:
Expand All @@ -853,7 +895,7 @@ def test(*args, **kwargs):
arg_gctime = gc_cumulative_time()
start = time.perf_counter()
try:
with ensure_free_stackframes():
with unwrap_exception_group(), ensure_free_stackframes():
result = self.test(*args, **kwargs)
finally:
finish = time.perf_counter()
Expand Down
54 changes: 54 additions & 0 deletions hypothesis-python/tests/cover/test_exceptiongroup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import sys
import asyncio

from trio.testing import RaisesGroup
from hypothesis import errors, given, strategies as st
import pytest

if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroup

def test_exceptiongroup():
@given(st.data())
def test_function(data):
async def task(pred):
return data.draw(st.booleans().filter(pred))

async def _main():
async with asyncio.TaskGroup() as tg:
tg.create_task(task(bool))
tg.create_task(task(lambda _: False))
asyncio.run(_main())
with pytest.raises(errors.Unsatisfiable):
test_function()

def test_exceptiongroup_nested():
@given(st.data())
def test_function(data):
async def task(pred):
return data.draw(st.booleans().filter(pred))

async def _main():
async with asyncio.TaskGroup():
async with asyncio.TaskGroup() as tg2:
tg2.create_task(task(bool))
tg2.create_task(task(lambda _: False))
asyncio.run(_main())
with pytest.raises(errors.Unsatisfiable):
test_function()


def test_exceptiongroup_user_originated():
@given(st.data())
def test_function(data):
raise ExceptionGroup("foo", [ValueError(), ValueError()])
with RaisesGroup(ValueError, ValueError, match="foo"):
test_function()


@given(st.data())
def test_single_exc_group(data):
raise ExceptionGroup("important message for user", [ValueError()])

with RaisesGroup(ValueError, match="important message for user"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test_single_exc_group()
1 change: 1 addition & 0 deletions requirements/test.in
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ attrs==24.1.0 # too early for https://github.com/python-attrs/attrs/pull/1329
pexpect
pytest
pytest-xdist
trio # for trio.testing.RaisesGroup, until pytest merges a version
Loading