--pdb treats KeyboardInterrupt as exception #3436
Conversation
_pytest/runner.py
Outdated
|
|
||
|
|
||
| class CallInfo(object): | ||
| """ Result/Exception info a function invocation. """ | ||
| #: None or ExceptionInfo object. | ||
| excinfo = None | ||
|
|
||
| def __init__(self, func, when): | ||
| def __init__(self, func, when, usepdb=None): |
There was a problem hiding this comment.
What about passing in the whole item?
Would make it more flexible for future changes.
There was a problem hiding this comment.
I don't mind changing it.
I figured it was cleaner to pass the minimum information I need to accomplish the task, passing more seemed like premature optimization.
There was a problem hiding this comment.
I personally prefer to pass just what you need as well. I would name the parameter differently as well, because it is more about treating KeyboardInterrupt as a special case... how about stop_on_keyboard_interrupt=True?
There was a problem hiding this comment.
Sounds good, but in either case we are going to stop on keyboard interrupt. Either we capture the exception or we re-raise it. How about calling it treat_keyboard_interrupt_as_exception?
_pytest/runner.py
Outdated
|
|
||
|
|
||
| class CallInfo(object): | ||
| """ Result/Exception info a function invocation. """ | ||
| #: None or ExceptionInfo object. | ||
| excinfo = None | ||
|
|
||
| def __init__(self, func, when): | ||
| def __init__(self, func, when, usepdb=None): |
There was a problem hiding this comment.
I personally prefer to pass just what you need as well. I would name the parameter differently as well, because it is more about treating KeyboardInterrupt as a special case... how about stop_on_keyboard_interrupt=True?
_pytest/runner.py
Outdated
| @@ -382,7 +385,8 @@ def __init__(self, longrepr, **extra): | |||
| def pytest_make_collect_report(collector): | |||
| call = CallInfo( | |||
| lambda: list(collector.collect()), | |||
| 'collect') | |||
| 'collect', | |||
| None) | |||
There was a problem hiding this comment.
We should treat this option as a bool, so this should be False; but I think we also want to stop on the debugger during collection, so we should pass the parameter here based on config.getvalue("usepdb").
There was a problem hiding this comment.
what is someone going to do with a debugger in the middle of runner.py?
I guess they could check the sys.path or something like that, but I'm worried about throwing them into a debugger in the middle of the pytest internals
| @@ -0,0 +1 @@ | |||
| The ``--pdb`` option now causes KeyboardInterrupt to enter the debugger, instead of stopping the test session. | |||
There was a problem hiding this comment.
Just to make sure: hitting CTRL+C again after landing on the debugger causes the debugger to exit right?
There was a problem hiding this comment.
Yes. I'll try to add a test that verifies that. Should I mention it in the changelog or docs?
There was a problem hiding this comment.
I think just mentioning it in the changelog is sufficient, something like:
... instead of stopping the test session. Hitting CTRL+C again exits the debugger.
There was a problem hiding this comment.
I see the test failed... not sure if it is easy to fix it; if not I'm OK with dropping the test, I think it is enough to ensure that we enter pdb when we get a KeyboardInterrupt.
There was a problem hiding this comment.
I also expected the test to fail because of pexpect's kill method not being cross-platform or something like that, but actually the test failed because pdb indeed behaves differently in python 3.6, and the KeyboardInterrupt does not cause it to exit. Are we ok with that?
There was a problem hiding this comment.
looks like it was an intentional change: https://bugs.python.org/issue7245.
Haha now I understand the discussion on the issue.
There was a problem hiding this comment.
The nosigint parameter to the Pdb constructor is supposed to disable that functionality, according to the docs (https://docs.python.org/3/library/pdb.html#pdb.Pdb). For some reason it doesn't seem to be working for me, but I can continue looking into it if we care about it.
On the other hand, probably better to let the pdb act like it usually acts for each version of python.
There was a problem hiding this comment.
On the other hand, probably better to let the pdb act like it usually acts for each version of python.
Definitely, I agree! Thanks for researching this. 👍
d950dde to
c258fe1
Compare
|
Awesome work @brianmaissy, thanks a lot! |
|
The py27-xdist failure on AppVeyor is unrelated so I think this is ready for merging. @RonnyPfannschmidt would you like to review this as well? |
Fixes #3299