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_keyboard_interrupt hook is also called with --maxfail #1865

Closed
blueyed opened this issue Aug 24, 2016 · 4 comments
Closed

pytest_keyboard_interrupt hook is also called with --maxfail #1865

blueyed opened this issue Aug 24, 2016 · 4 comments
Labels
type: enhancement new feature or API change, should be merged into features branch

Comments

@blueyed
Copy link
Contributor

blueyed commented Aug 24, 2016

When using -x / --maxfail the Interrupted exception is thrown, and gets handled in the same place as KeyboardInterrupt (

pytest/_pytest/main.py

Lines 99 to 106 in 86ec3f3

except KeyboardInterrupt:
excinfo = _pytest._code.ExceptionInfo()
if initstate < 2 and isinstance(
excinfo.value, pytest.exit.Exception):
sys.stderr.write('{0}: {1}\n'.format(
excinfo.typename, excinfo.value.msg))
config.hook.pytest_keyboard_interrupt(excinfo=excinfo)
session.exitstatus = EXIT_INTERRUPTED
), because it is a subclass.

Therefore I think the hook name is confusing / misleading.
If this behavior is intentional, it should get added to the documentation at least.

I have noticed this, because pytest-testmon uses this hook to not save its data: https://github.com/tarpas/pytest-testmon/blob/55c89bb3df65c94e226ead8dd465f3dd5bb8927b/testmon/pytest_testmon.py#L173-L174
Here a check using excinfo.typename could be added to handle 'Interrupted' different from 'KeyboardInterrupt'.

btw: is there a way to import the Interrupted class from pytest?

@The-Compiler
Copy link
Member

FWIW, Interrupted being a subclass of KeyboardInterrupt sounds weird to me...

blueyed added a commit to blueyed/testmon that referenced this issue Aug 24, 2016
When using `-x` to abort the test run on the first failure,
pytest-testmon would previously not store the collected data: the
`pytest_keyboard_interrupt` hook that is used for this gets also called
for pytest's internal `Interrupted` exception, which is a subclass of
`KeyboardInterrupt` (pytest-dev/pytest#1865).

This patch changes it to use the result in the `pytest_runtest_protocol`
method to check for `KeyboardInterrupt` there explicitly.

This makes the `pytest_keyboard_interrupt` obsolete, but I've left it
with some asserts for now.
@nicoddemus
Copy link
Member

I agree. I think that we should:

  1. Subclass Interrupted from something else (RuntimeError?)
  2. Make Interrupted part of the public API so plugins can raise or catch it; perhaps by making it available in the pytest namespace? Also, how about renaming it to SessionInterrupt?

This has to be done carefully, because there are a number of places which handle KeyboardInterrupt on pytest's code base that will have to be changed to also handle SessionInterrupt.

I also noticed that Exit also subclasses KeyboardInterrupt.

@The-Compiler
Copy link
Member

  1. Subclass Interrupted from something else (RuntimeError?)

If you do except RuntimeError:, would you expect Interrupted to be caught (I don't)? If not, it should simply subclass Exception.

@nicoddemus
Copy link
Member

Good point!

@nicoddemus nicoddemus added the type: enhancement new feature or API change, should be merged into features branch label Aug 26, 2016
blueyed added a commit to blueyed/pytest that referenced this issue Nov 2, 2018
…errupt

This is required for properly getting out of pdb, where
KeyboardInterrupt is caught in py36 at least.

Ref: pytest-dev#1865 (comment)
blueyed added a commit to blueyed/pytest that referenced this issue Dec 9, 2018
…errupt

This is required for properly getting out of pdb, where
KeyboardInterrupt is caught in py36 at least.

Ref: pytest-dev#1865 (comment)
blueyed added a commit to blueyed/pytest that referenced this issue Dec 10, 2018
…errupt

This is required for properly getting out of pdb, where
KeyboardInterrupt is caught in py36 at least.

Ref: pytest-dev#1865 (comment)
blueyed added a commit to blueyed/pytest that referenced this issue Dec 11, 2018
…errupt

This is required for properly getting out of pdb, where
KeyboardInterrupt is caught in py36 at least.

Ref: pytest-dev#1865 (comment)
blueyed added a commit to blueyed/pytest that referenced this issue Dec 11, 2018
…errupt

This is required for properly getting out of pdb, where
KeyboardInterrupt is caught in py36 at least.

Ref: pytest-dev#1865 (comment)
@blueyed blueyed closed this as completed Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

3 participants