Opened 10 years ago
Last modified 9 years ago
#5795 enhancement new
Failure's startDebugMode should allow using a debugger other than pdb
Reported by: | Julian Berman | Owned by: | Julian Berman |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Jonathan Lange | Branch: |
branches/failure-select-debugger-5795
branch-diff, diff-cov, branch-cov, buildbot |
Author: | julian, tomprince |
Description (last modified by )
Following #5794, Failure
's startDebugMode
function needs to be updated to take a debugger
argument with a debugger it will use to do post-mortems.
Attachments (5)
Change History (27)
comment:1 Changed 10 years ago by
Cc: | Jonathan Lange added |
---|
comment:2 Changed 10 years ago by
Component: | trial → core |
---|---|
Description: | modified (diff) |
comment:3 Changed 9 years ago by
comment:4 follow-up: 5 Changed 9 years ago by
What if the debugger the user selected doesn't have post-mortem functionality?
comment:5 Changed 9 years ago by
Replying to exarkun:
What if the debugger the user selected doesn't have post-mortem functionality?
In that case falling back to pdb would make sense; but pudb and pdbpp both have post-mortem functionality and those are the ones that I think people are generally most interested in. I suspect that if a new one arose, API-compatibility with pdb would be a goal for them too.
comment:6 Changed 9 years ago by
Keywords: | review added |
---|
OK. Here's a patch.
This patch also introduces failure.stopDebugMode
, which I've more than once needed to do cleanup in tests, including here.
Also, I should mention for reviewers that the debugger that most of us probably use who care about this (pudb) seems to have a bug? whereby it doesn't satisfy pdb's interface (it takes a tuple of exc_info
rather than a traceback, which I've filed as https://github.com/inducer/pudb/issues/70 until I get a chance to look closer, so until that's fixed pudb won't actually work unless you make that change locally). pdbpp of course works, but that didn't even require this patch obviously.
Changed 9 years ago by
Attachment: | startdebuggermode.patch added |
---|
Changed 9 years ago by
Attachment: | startdebuggermode.2.patch added |
---|
comment:8 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/failure-select-debugger-5795 |
(In [38007]) Branching to failure-select-debugger-5795.
comment:9 Changed 9 years ago by
comment:10 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Julian Berman |
debugger
needs a@param
.- I wonder if having _realInit and _debugInit on
Failure
would make sense, instead of what is done currently. - I think the new default for
debugger
is incorrect. opt_debugger
probably shouldn't have a docstring (see #6427).- Might it make more sense for startDebugMode to take the post-mortem function, rather than the debugger module (since that is what it cares about?
- That would mean the logic for determining whether the debugger supports post-mortems gets moved to trial.
- If there is a debugger that doesnt support
.post_mortem
(or somebody wants to something different with the traceback), it would then be easy to add an option to call something arbitrary there.
- Is there a reason for the local import in
test_setsUpFailureDebugMode
? stopDebugMode
is not tested.
comment:11 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Julian Berman deleted |
OK. Phew.
- Done
- Like this, forgot why I didn't do it before, but did it now.
- Not sure what you mean, but as you'll see this patch takes a slightly different approach due to #5, so let me know if this still applies.
- Done.
- Yes, like this. Done.
- Yes, CapturingDebugger being mutated, but I fixed that with a not totally ideal but slightly more reasonable thing that still allows putting a debugger someplace where command line can find it.
- Done.
One thing I removed entirely here is _wrappedPdb
, a function that was loading a local .pdbrc
and readline
. Both of those are present in the standard library's pdb
now as of 2.6, so we don't need to do this anymore. (Besides the fact that it didn't actually work for this part of this patch, because pdb.post_mortem
which was what failure.startDebugMode
was directly calling, has no way to inject a custom pdb.Pdb
instance).
Changed 9 years ago by
Attachment: | startdebuggermode.3.patch added |
---|
comment:12 Changed 9 years ago by
Sorry, inadvertently renamed an attribute on CapturingDebugger
which broke two runner tests, fixed those in an incremental patch. I may flesh out CapturingDebugger later since there are other debugger related tickets that I may eventually get to.
Changed 9 years ago by
Attachment: | fixCapturingDebugger.patch added |
---|
comment:13 Changed 9 years ago by
Owner: | set to Tom Prince |
---|
comment:14 Changed 9 years ago by
comment:15 Changed 9 years ago by
comment:16 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Julian Berman |
- It appears there are still a couple of test failures.
- Are there existing debuggers that raise
BdbQuit
? Looking at the code forbdb
, it seems more likeBdbQuit
is used for internal communication, rather than something that is supposed to escape.- In fact, I'm not entirely sure why you are falling back to
pdb
here anyway?
- In fact, I'm not entirely sure why you are falling back to
- In the news file:
failure.startDebugMode
doesn't respect--debugger
, trial callsstartDebugMode
in a way that respects--debugger
. test_stopDebugMode
is missing a docstring- I wonder if replacing
createAndCleanup
with something liketwisted.python.test.modules_helpers.replaceSysModules
might be simpler?- This means you can use a shorter name for the debugger.
test_noPostMortemMethod
shouldn't useCapturingDebugger
. (I dislike the mutation necessary to use it here.
comment:17 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Julian Berman deleted |
- Fixed
- AFAIU they all do I think.
pdb
andpudb
both do specifically, if you pressq
or otherwise quit. They're not wrapping anything special, they just pass through the quit process to bdb. I fixed / removed the fallback though, not sure what the thought process was there when I wrote it. - Fixed
- Fixed
- Nice.
- Fixed.
Thanks for the review :)
Changed 9 years ago by
Attachment: | startdebugmode.patch added |
---|
comment:18 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Julian Berman |
- Testing with the following patch applied prints out "Hi Here" which suggest that
BdbQuit
doesn't escapepdb.post_mortem
.-
twisted/python/failure.py
diff --git twisted/python/failure.py twisted/python/failure.py index 5c7ffd3..1a11160 100644
class Failure: 353 353 try: 354 354 postMortem(exc[2]) 355 355 except bdb.BdbQuit: 356 print "Hi There" 356 357 pass 358 else: 359 print "Hi Here" 357 360 358 361 self._realInit(exc_value, exc_type, exc_tb, captureVars) 359 362
-
- Please use reflect.safe_str, rather than hand coding it (in
_debugInit
).
Otherwise, this looks good. Please resubmit for review after addressing the above points.
comment:19 Changed 9 years ago by
Hey. Thanks.
I switched to safe_str.
For 1, if you run:
from twisted.trial import unittest class TestFoo(unittest.TestCase): def test_thing(self): foo = 12
with trial --debug
and get to the test body, and then quit (with q), you will see that a {{{bdb.BdbQuit}} exception is raised and gets all the way out of the debugger.
Failure's startDebugMode then will kick in, and you get
Jumping into debugger for post-mortem of exception '' > /usr/local/Cellar/python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/bdb.py(68)dispatch_line() -> if self.quitting: raise BdbQuit
Which you will have to quit a second time.
Apparently though what I had doesn't fix it since you get that even with the except there, so I have to investigate when I'm more awake.
comment:20 Changed 9 years ago by
Without doing any investigating, that seems like it isn't the post-mortem triggering that, but rather the other debugger bit.
With:
from twisted.trial.unittest import TestCase from twisted.python.failure import Failure class Test(TestCase): def test_test(self): try: raise Exception except: Failure()
If I 'c' from the first prompt, which continues until the post-mortem is activated. Quitting from there doesn't appear to lead to an exception.
comment:21 Changed 9 years ago by
Author: | tomprince → julian, tomprince |
---|---|
Branch: | branches/failure-select-debugger-5795 → branches/failure-select-debugger-5795-2 |
(In [38961]) Branching to failure-select-debugger-5795-2.
comment:22 Changed 9 years ago by
Branch: | branches/failure-select-debugger-5795-2 → branches/failure-select-debugger-5795 |
---|
#6276 was a duplicate of this.