Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5794 enhancement closed fixed (fixed)

Allow Trial to use debuggers other than pdb

Reported by: Julian Berman Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Jonathan Lange Branch: branches/trial-with-other-debuggers-5794
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description (last modified by Thijs Triemstra)

Trial should accept an arbitrary debugger to use for debugging when the --debug option is passed.

Attachments (7)

debuggers.patch (6.5 KB) - added by Julian Berman 9 years ago.
debuggers-take-2.patch (9.4 KB) - added by Julian Berman 9 years ago.
updated_trial_debug.patch (8.5 KB) - added by Ying Li 9 years ago.
debuggers-take-3.patch (12.2 KB) - added by Julian Berman 9 years ago.
debuggers-take-4.patch (12.5 KB) - added by Julian Berman 9 years ago.
debuggers-take-5.patch (12.6 KB) - added by Julian Berman 9 years ago.
debugger-not-found-exception.patch (1.5 KB) - added by Julian Berman 9 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

Changed 9 years ago by Julian Berman

Attachment: debuggers.patch added

comment:2 Changed 9 years ago by Julian Berman

There's one other hard-coded use of pdb in twisted.python.failure (that can also cause pdb to be run even when passing --debugger somethingelse if something fails during a Failure's init) that needs to be updated. I've put that in #5795.

comment:3 Changed 9 years ago by Thijs Triemstra

Description: modified (diff)

Hi Julian, thanks for your patch. More options is good but your patch doesn't give any examples of which other debuggers to use, or how to use them. An addition to the howto/examples and man page would be helpful there. An arbitrary debugger is a bold statement.

Changed 9 years ago by Julian Berman

Attachment: debuggers-take-2.patch added

comment:4 Changed 9 years ago by Julian Berman

Added to man page, and a paragraph on using debuggers, which I didn't see mentioned previously at all. New patch is debuggers-take-2.patch.

Thanks.

comment:5 Changed 9 years ago by Ying Li

Owner: set to Ying Li

comment:6 Changed 9 years ago by Ying Li

Keywords: review removed
Owner: changed from Ying Li to Julian Berman

Thanks for working on this Julian! PuDB support would be awesome!

  1. Could you please add a news file describing what this patch does? (see http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles for what one looks like and where it'd go)
  1. There should be 2 new lines between class method definitions (see test_runner.py), as per the twisted coding standard
  1. There is no test for the case where a debugger is not passed (the default debugger used is pdb) and the case where the debugger specified is pdb (that it gets a wrapped PDB instance)
  1. It seems kind of odd to pass either a string or a debugger to the runner. Perhaps only the string should be passed - since the runner needs to get a wrapped PDB instance anyway, reflect could also be called there.
  1. It would be nice if, if pudb is not installed or the user specifies some other debugger that doesn't exist, that a friendly error message is presented to the user rather than a reflect traceback saying that there is no module named 'pudb'.

This patch is also out of date with trunk - I've tried to manually merge it in - I've included the code I used to test this (and it works!) but I may have done it wrong.

Changed 9 years ago by Ying Li

Attachment: updated_trial_debug.patch added

comment:7 Changed 9 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

Hey! Thanks for the review(s) :).

Agreement all around on all points, hope I've addressed them satisfactorily.

Didn't see *exactly* which test method my spacing was wrong on -- hope you meant that you just wanted the CapturingDebugger line also spaced by 2 lines, if not please let me know.

Thanks again (and sorry about sucking with git-svn).

Changed 9 years ago by Julian Berman

Attachment: debuggers-take-3.patch added

Changed 9 years ago by Julian Berman

Attachment: debuggers-take-4.patch added

comment:8 Changed 9 years ago by Julian Berman

Whoops, forgot that diff isn't going to include new files by default, so it left out the topfile. Sorry 'bout the noise.

Changed 9 years ago by Julian Berman

Attachment: debuggers-take-5.patch added

comment:9 Changed 9 years ago by Ying Li

Author: cyli
Branch: branches/trial-with-other-debuggers-5794

(In [36225]) Branching to 'trial-with-other-debuggers-5794'

comment:10 Changed 9 years ago by Ying Li

(In [36226]) Apply debuggers-take-5.patch

refs #5794

comment:11 Changed 9 years ago by Ying Li

Owner: set to Julian Berman

Hi Julian!

Thanks again for working on this, and sorry the response has taken so long! This looks pretty great - just a few comments:

  1. There is a problem with twisted.trial.test_script.TestRun.test_debugger_not_found when trial is run with a --reactor parameter. (see buildbot failures).

Since you're calling trial.run, it is re-parsing the arguments and trying to set the reactor twice. You might want to patch up the config parser to not do so.

  1. Also a test that asserts that an invalid debugger raises the _DebuggerNotFound would be good, since that seems to the one step in that process that is missing coverage.

Thank you again for all your hard work!

comment:12 Changed 9 years ago by Ying Li

Keywords: review removed

comment:13 Changed 9 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

Thanks for thorough reviews :).

Think this addresses the two issues.

Once again the patch is incremental.

Changed 9 years ago by Julian Berman

comment:14 Changed 9 years ago by Ying Li

(In [36227]) Apply debugger-not-found-exception.patch

refs #5794

comment:15 Changed 9 years ago by Ying Li

Keywords: review removed

Tests all seem to pass on buildbot. Thanks again for all your work Julian! Merging now.

comment:16 Changed 9 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [36228]) Merge trial-with-other-debuggers-5794: Allow Trial to use debuggers other than pdb

Author: Julian Reviewer: cyli Fixes: #5794

Add a --debugger option to trial that allows the user to specify which debugger to use if --debug is passed.

comment:17 Changed 9 years ago by Jean-Paul Calderone

There's at least one variable in this new code that doesn't follow the naming convention: runcall_called. Please make sure you're familiar with the coding standard and apply it to all code you write and review for Twisted.

Also, I would be nice to see two other follow-up improvements related to this code:

  1. What is the interface required of other debuggers used with this feature? The prose documentation implies very vaguely that debuggers should be "similar" to pdb. What does that mean though? There should be an actual interface specified.
  2. Twisted has a plugin system. It would be nice to use it here (trial already uses plugins for some of its other extensible features).
Note: See TracTickets for help on using tickets.