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

Pytest should by default terminate on exceptions that inherit from BaseException and related changes to exception handling #3702

Closed
ceridwen opened this issue Jul 21, 2018 · 4 comments · Fixed by #12191
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@ceridwen
Copy link
Contributor

ceridwen commented Jul 21, 2018

At the moment if I use the signal module to catch SIGTERM and raise an exception, like the Python interpreter does by default for SIGINT and KeyboardInterrupt, sending SIGTERM will only cause one test to fail: pytest will catch the exception and continue running tests. This seems like the wrong behavior to me: there should be way to raise an exception that pytest won't continue on that isn't KeyboardInterrupt.

The reason I want to do this is that Jenkins, when it aborts a job, seems to send SIGTERM to its subprocesses, and when I abort a job, I almost always want the test clean-up code to run. I could call pytest.exit, except that then the signal handler is only useful when pytest itself is running, not when other Python code is running. I could write two signal handlers, one for pytest and one for everything else and then try to make sure that the correct signal handler is always set (note that the pytest signal handler also has to ensure that clean up is run outside pytest as well), or do stack introspection in the signal handler to try to determine if pytest is running, but both of those solutions look fragile and hard to get right; complexity is the last thing I want in my signal-handling code. I could raise KeyboardInterrupt, but KeyboardInterrupt when directly raised in code that pytest is executing causes pytest to terminate without running clean-up, unlike when SIGINT is sent to the Python interpreter, which defeats the purpose. This would also overload KeyboardInterrupt, which seems like bad style to me: KeyboardInterrupt should mean SIGINT, not other termination signals.

It seems to me the natural way to get Python to do clean-up correctly when handling SIGTERM is the way KeyboardInterrupt is handled by default, raise an exception. Thus, I'd propose that:

  1. pytest should terminate on exceptions that inherit from BaseException, except for GeneratorExit. (This doesn't normally terminate the interpreter.) Of the built-in exceptions, this means only SystemExit and KeyboardInterrupt.

  2. pytest.raises should remain able to catch SystemExit, KeyboardInterrupt, and other exceptions that inherit from BaseException and not terminate, so there's still a way to test expected instances of those exceptions, while allowing unexpected instances to cause pytest to exit.

  3. pytest should allow clean-up code to run when KeyboardInterrupt is raised in Python code rather than via a signal for consistency in behavior.

(All my testing was done on pytest 3.6.0).

@ceridwen ceridwen added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Jul 21, 2018
@pytestbot pytestbot added the type: bug problem that needs to be addressed label Jul 21, 2018
@asottile
Copy link
Member

The hard (impossible) part here is this should be a failing test suite:

def test_that_fails():
    assert False

def test_oops_i_called_exit():
    exit(0)

given the proposal, I don't see a way to keep that from silently passing

@ceridwen
Copy link
Contributor Author

I was imprecise in my terminology: pytest does catch KeyboardInterrupt, but immediately generates the report without failing the currently running test when that exception is thrown. I'm looking for that behavior rather than for pytest to exit without a report. I will point out that it's already possible to create that test suite though:

import os

def test_that_fails():
    assert False

def test_oops_i_called_exit():
    os._exit(1)

@asottile asottile removed the type: bug problem that needs to be addressed label Jul 22, 2018
@ceridwen ceridwen changed the title Pytest should not by default catch exceptions that inherit from BaseException and related changes to exception handling Pytest should by default terminate on exceptions that inherit from BaseException and related changes to exception handling Jul 24, 2018
@ceridwen
Copy link
Contributor Author

I've reworded some aspects of the proposal to make it clearer that I'm not suggesting that pytest not handle the exception at all when an exception inheriting from BaseException is raised, but rather interrupt the tests at that point and stop running the rest of the suite.

@pytest-dev pytest-dev deleted a comment from pytestbot Oct 31, 2019
@anitahammer
Copy link
Contributor

Not reacting to keyboard interrupts is a calling card of pytest, however it seems like it's over #12191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants