Closed Bug 521457 Opened 11 years ago Closed 11 years ago
Add debugger options to runxpcshelltests
.py and runreftest .py
Add the --debugger and --debugger-args command-line options to runxpcshelltests.py in order to support running xpcshell tests under valgrind.
This patch adds --debugger and --debugger-args to runxpcshelltests.py. Verified locally.
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.
Consolidate most of the debugger option code into automationutils.py.
Summary: Add debugger options to runxpcshelltests.py → Add debugger options to runxpcshelltests.py and runreftest.py
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-
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?
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".
Updated patch which addresses previous review comments.
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+
code comment in comment #9 fixed, and patch pushed as http://hg.mozilla.org/mozilla-central/rev/0d4182d52170
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.