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 SystemExit? #2223

Closed
aarchiba opened this issue Nov 23, 2019 · 5 comments · Fixed by #3107
Closed

How should we handle SystemExit? #2223

aarchiba opened this issue Nov 23, 2019 · 5 comments · Fixed by #3107
Labels
enhancement it's not broken, but we want it to be better

Comments

@aarchiba
Copy link
Contributor

PR #2189 demonstrates that we handle SystemExit (and GeneratorExit) in slightly inconsistent ways - if raised from inside a strategy it is handled like a normal exception; but if raised from within a test function it is captured and treated like KeyboardInterrupt in some ways, ensuring for example that it doesn't trigger "Flaky" if raised some of the time but not others. What is the correct behaviour?

@Zac-HD Zac-HD added the opinions-sought tell us what you think about these ones! label Nov 24, 2019
@aarchiba
Copy link
Contributor Author

I think it makes sense to treat SystemExit and GeneratorExit like we do normal exceptions when raised from both strategies and test functions. This requires some changes, as when they arise from test functions they are currently handled more like KeyboardInterrupt, in particular not triggering Flaky if raised some times and not others.

@DRMacIver
Copy link
Member

I feel like SystemExit and GeneratorExit are slightly different things: Someone calling sys.exit probably meant it, while a raised GeneratorExit is probably a bug (because neither tests nor strategies are coroutines - they might wrap coroutines, but that wrapper should be handling the exit).

This doesn't necessarily mean that they should be handled differently though.

I do think they're fundamentally different from KeyboardInterrupt, which can basically be triggered at any time and doesn't mean much, while these should happen predictably.

My inclination would be that SystemExit and KeyboardInterrupt should always be propagated but that we should probably treat GeneratorExit like any other exception. I'm not strongly wedded to this view though, and might be missing something about GeneratorExit.

@aarchiba
Copy link
Contributor Author

Well, one way GeneratorExit might appear is, if I recall correctly, nose uses (used?) generators to implement parametrized tests - you'd write a generator that yielded sets of arguments. If your test runner bailed out without running all the tests, that generator would receive a GeneratorExit as it exited "out the side" without finishing.

But my reasoning was just that: both GeneratorExit and SystemExit are emitted deterministically by code, so checking for flakiness in their emission makes sense.

The remaining question is whether a function under test should be able to actually exit from hypothesis testing with sys.exit(), but already pytest intercepts and reports that more or less like a normal exception (and does not exit to the OS when it appears).

@Zac-HD
Copy link
Member

Zac-HD commented Nov 25, 2019

nose uses (used?) generators to implement parametrized tests

@given raises an error if the wrapped function returns anything but None, so we don't need to worry about this at least!

IMO "SystemExit ought to be deterministic and pytest treats it as a normal exception" is enough to justify us doing the same.

@Zac-HD Zac-HD added enhancement it's not broken, but we want it to be better and removed opinions-sought tell us what you think about these ones! labels Nov 18, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Nov 18, 2020

In principle this can be solved by adding GeneratorExit and SystemExit to the list here:

def failure_exceptions_to_catch():
"""Return a tuple of exceptions meaning 'this test has failed', to catch.
This is intended to cover most common test runners; if you would
like another to be added please open an issue or pull request.
"""
exceptions = [Exception]
if "_pytest" in sys.modules: # pragma: no branch
exceptions.append(sys.modules["_pytest"].outcomes.Failed)
return tuple(exceptions)

And then writing a test that e.g.

@given(st.integers())
def test(x):
    if x > 100:
        raise SystemExit

prints output including Falsifying example: and x=101... which at the moment it doesn't. Unclear why so this isn't quite a "good first issue", but probably suitable for a first contribution to Hypothesis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants