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

Change in behaviour with multiple examples and pytest.skip #3453

Closed
hauntsaninja opened this issue Sep 8, 2022 · 3 comments · Fixed by #3455
Closed

Change in behaviour with multiple examples and pytest.skip #3453

hauntsaninja opened this issue Sep 8, 2022 · 3 comments · Fixed by #3455
Labels
interop how to play nicely with other packages

Comments

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Sep 8, 2022

There is a change in behaviour between hypothesis 6.54.1 and 6.54.2.

This change is for the better, I think, but just wanted to confirm it was expected, since it doesn't obviously line up with the only changelog entry:

This patch fixes our workaround for a pytest bug where the inner exceptions in an ExceptionGroup are not displayed (issue #3430).

Change wasn't obvious to me at a skim from https://github.com/HypothesisWorks/hypothesis/compare/hypothesis-python-6.54.1..hypothesis-python-6.54.2 either, but I don't know too much about pytest internals

Consider a hypothesis test that has multiple examples which runs pytest.skip (I'm aware that this is perhaps a little unidiomatic):

import hypothesis
import hypothesis.strategies as st

import pytest

@hypothesis.given(value=st.text())
@hypothesis.example("hello")
@hypothesis.example("goodbye")
def test_skip(value):
    pytest.skip("TODO: fix this test")

With 6.54.1, you get:

 exceptiongroup.BaseExceptionGroup: Hypothesis found 2 distinct failures in explicit examples. (2 sub-exceptions)

With 6.54.2, you get a skipped test.

In both versions, if you have only a single example, you get a skipped test.

If appropriate, I'd also be happy to add a regression test to the test suite.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 9, 2022

...ah, I see what's happening here now, but it's certainly not obvious.

  • Switch from MultipleFailures to PEP-654 ExceptionGroup #3308 switched over to using ExceptionGroup, and also adds a forced report_multiple_bugs=False setting if you're using Pytest but not --tb=native (because in that case the inner exceptions weren't rendered)
  • v6.54.2 fixes the detection, so that we actually do stop after discovering the first exception. When Hypothesis re-raises a Skipped exception, Pytest skips the test, but there's no special handling for exception groups.
  • For generated examples, we explicitly stop generating at the first 'skip' exception and immediately reraise. For consistency, I think that we should do the same for explicit examples, which means reverting to the behaviour of 6.54.1. Here's a diff:
@@ -437,7 +437,11 @@ def execute_explicit_examples(state, wrapped_test, arguments, kwargs, original_s
                     err = new

                 yield (fragments_reported, err)
-                if state.settings.report_multiple_bugs and pytest_shows_exceptiongroups:
+                if (
+                    state.settings.report_multiple_bugs
+                    and pytest_shows_exceptiongroups
+                    and not isinstance(err, skip_exceptions_to_reraise())
+                ):
                     continue
                 break
             finally:
@@ -1192,6 +1196,13 @@ def given(
                 # If we're not going to report multiple bugs, we would have
                 # stopped running explicit examples at the first failure.
                 assert len(errors) == 1 or state.settings.report_multiple_bugs
+
+                # If an explicit example raised a 'skip' exception, ensure it's never
+                # wrapped up in an exception group.  Because we break out of the loop
+                # immediately on finding a skip, if present it's always the last error.
+                if isinstance(errors[-1], skip_exceptions_to_reraise()):
+                    del errors[:-1]
+
                 _raise_to_user(errors, state.settings, [], " in explicit examples")

             # If there were any explicit examples, they all ran successfully.

If you'd like to add a regression test and make a PR, go ahead! Otherwise I'll probably get to it next week, after PyBay 🙂

@Zac-HD Zac-HD added the interop how to play nicely with other packages label Sep 9, 2022
@hauntsaninja
Copy link
Contributor Author

Thanks for looking into this! :-)

@Zac-HD
Copy link
Member

Zac-HD commented Sep 20, 2022

Thanks for the report! Wouldn't have (couldn't have) fixed it without you 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop how to play nicely with other packages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants