Skip to content

Derive outcomes.exit.Exit from SystemExit instead of KeyboardInterrupt #4292

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

Merged
merged 1 commit into from
Dec 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions changelog/4292.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
``pytest.outcomes.Exit`` is derived from ``SystemExit`` instead of ``KeyboardInterrupt``.
2 changes: 1 addition & 1 deletion src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def wrap_session(config, doit):
raise
except Failed:
session.exitstatus = EXIT_TESTSFAILED
except KeyboardInterrupt:
except (KeyboardInterrupt, exit.Exception):
Copy link
Member

Choose a reason for hiding this comment

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

this implies that this is a breaking change for externals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest?
Should we have a custom exception that is still derived from KeyboardInterrupt (if possible)?

Copy link
Member

Choose a reason for hiding this comment

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

@blueyed its not clear to me off hand, i wonder if having multiple base classes is acceptable while preparing for a breaking release (i dont see a way to deal out warnings about this one tho)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.
I've just noticed that also deriving from KeyboardInterrupt goes against what this tries to do: pdb not catching pytest's Exit.

I think it is OK for a feature release, and plugins should really catch/handle Exit in the first place already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related btw:

class Interrupted(KeyboardInterrupt):

Copy link
Member

Choose a reason for hiding this comment

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

I think it is OK for a feature release, and plugins should really catch/handle Exit in the first place already.

I think so too. I don't think this will bring problems to anyone TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus
Approve it then maybe?

(I am waiting for this for more than a month already for #4280)

excinfo = _pytest._code.ExceptionInfo.from_current()
exitstatus = EXIT_INTERRUPTED
if initstate <= 2 and isinstance(excinfo.value, exit.Exception):
Expand Down
6 changes: 3 additions & 3 deletions src/_pytest/outcomes.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,21 @@ class Failed(OutcomeException):
__module__ = "builtins"


class Exit(KeyboardInterrupt):
class Exit(SystemExit):
""" raised for immediate program exits (no tracebacks/summaries)"""

def __init__(self, msg="unknown reason", returncode=None):
self.msg = msg
self.returncode = returncode
KeyboardInterrupt.__init__(self, msg)
SystemExit.__init__(self, msg)


# exposed helper methods


def exit(msg, returncode=None):
"""
Exit testing process as if KeyboardInterrupt was triggered.
Exit testing process as if SystemExit was triggered.

:param str msg: message to display upon exit.
:param int returncode: return code to be used when exiting pytest.
Expand Down
8 changes: 5 additions & 3 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from .reports import CollectReport
from .reports import TestReport
from _pytest._code.code import ExceptionInfo
from _pytest.outcomes import Exit
from _pytest.outcomes import skip
from _pytest.outcomes import Skipped
from _pytest.outcomes import TEST_OUTCOME
Expand Down Expand Up @@ -190,10 +191,11 @@ def check_interactive_exception(call, report):
def call_runtest_hook(item, when, **kwds):
hookname = "pytest_runtest_" + when
ihook = getattr(item.ihook, hookname)
reraise = (Exit,)
if not item.config.getvalue("usepdb"):
reraise += (KeyboardInterrupt,)
return CallInfo.from_call(
lambda: ihook(item=item, **kwds),
when=when,
reraise=KeyboardInterrupt if not item.config.getvalue("usepdb") else (),
lambda: ihook(item=item, **kwds), when=when, reraise=reraise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth a # fmt: off / # fmt: on block maybe to silence black here? (if this code stays like this)

Copy link
Member

Choose a reason for hiding this comment

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

IMHO that's fine 😁

)


Expand Down
2 changes: 1 addition & 1 deletion testing/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ def test_outcomeexception_passes_except_Exception():
def test_pytest_exit():
with pytest.raises(pytest.exit.Exception) as excinfo:
pytest.exit("hello")
assert excinfo.errisinstance(KeyboardInterrupt)
assert excinfo.errisinstance(pytest.exit.Exception)


def test_pytest_fail():
Expand Down