Closed
Bug 521457
Opened 12 years ago
Closed 12 years ago
Add debugger options to runxpcshelltests.py and runreftest.py
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jgriffin, Assigned: jgriffin)
References
Details
Attachments
(1 file, 2 obsolete files)
17.09 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
Add the --debugger and --debugger-args command-line options to runxpcshelltests.py in order to support running xpcshell tests under valgrind.
Assignee | ||
Comment 1•12 years ago
|
||
This patch adds --debugger and --debugger-args to runxpcshelltests.py. Verified locally.
Attachment #405536 -
Flags: review?(ted.mielczarek)
Comment 2•12 years ago
|
||
If you're going to support these in all of (runtests.py, runreftest.py, runxpcshelltests.py), please move parts of it down into automationutils.py, which is shared by all three. See addCommonOptions: http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#51 The DEBUGGER_INFO variable could get moved into automationutils as well. |searchPath| would probably be hard to move, since it relies on knowing the script's previous working directory.
Assignee | ||
Comment 3•12 years ago
|
||
Consolidate most of the debugger option code into automationutils.py.
Attachment #405536 -
Attachment is obsolete: true
Attachment #405607 -
Flags: review?(ted.mielczarek)
Attachment #405536 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Summary: Add debugger options to runxpcshelltests.py → Add debugger options to runxpcshelltests.py and runreftest.py
Comment 5•12 years ago
|
||
Comment on attachment 405607 [details] [diff] [review] patch v2 diff --git a/build/automationutils.py b/build/automationutils.py +def getFullPath(directory, path): + "Get an absolute path relative to cwd." + return os.path.normpath(os.path.join(directory, os.path.expanduser(path))) Fix the comment to make it clear that it resolves paths relative to the |directory| argument. diff --git a/layout/tools/reftest/runreftest.py b/layout/tools/reftest/runreftest.py +from automationutils import addCommonOptions, processLeakLog, DEBUGGER_INFO, \ + getDebuggerInfo I'd rather you either: 1) Just run past 80 characters or 2) from automationutils import * using a line continuation here just looks ugly. + parser.add_option("--debugger-interactive", + action = "store_true", dest = "debuggerInteractive", + help = "prevents the test harness from redirecting " + "stdout and stderr for interactive debuggers") Why not just move this to addCommonOptions as well? (Also, I think this option is unnecessary, I'm not sure why bent added it in the first place, but I guess just leave it for now.) diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py + if debuggerInfo["interactive"]: + options.interactive = true I don't think you necessarily want to do this. Running with --debugger=gdb will drop you to a gdb prompt, from which you can set breakpoints and run. --interactive is useful for interactively running parts of a test, but I don't think it ought to be forced on by --debugger. You may need to amend the bits where we decide whether to pipe stdout or not to know if we are using a debugger. r- for those few things, but overall it looks pretty good.
Attachment #405607 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 6•12 years ago
|
||
Thanks for the review ted. Regarding --debugger-interactive, I didn't add it to common opts because runxpcshelltests.py already has an --interactive flag. Do we want both --interactive and --debugger-interactive there?
Comment 7•12 years ago
|
||
If we're going to keep it, then yes, we should have it for both. --debugger-interactive apparently means "my debugger is interactive, so don't capture stdout", where xpcshell's --interactive means "don't auto-run the test".
Assignee | ||
Comment 8•12 years ago
|
||
Updated patch which addresses previous review comments.
Attachment #405607 -
Attachment is obsolete: true
Attachment #405949 -
Flags: review?(ted.mielczarek)
Comment 9•12 years ago
|
||
Comment on attachment 405949 [details] [diff] [review] patch v3 +def getFullPath(directory, path): + "Get an absolute path relative to cwd." + return os.path.normpath(os.path.join(directory, os.path.expanduser(path))) Still need to fix that comment. r=me with that nit fixed.
Attachment #405949 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 10•12 years ago
|
||
code comment in comment #9 fixed, and patch pushed as http://hg.mozilla.org/mozilla-central/rev/0d4182d52170
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•