521457 - Add debugger options to runxpcshelltests.py and runreftest.py

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

Add the --debugger and --debugger-args command-line options to runxpcshelltests.py in order to support running xpcshell tests under valgrind.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: patch

This patch adds --debugger and --debugger-args to runxpcshelltests.py.  Verified locally.

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Jonathan Griffin (:jgriffin)]

Attachment: patch v2

Consolidate most of the debugger option code into automationutils.py.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Jonathan Griffin (:jgriffin)]

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

[(not currently active) Ted Mielczarek]

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".

Comment 8

[Jonathan Griffin (:jgriffin)]

Attachment: patch v3

Updated patch which addresses previous review comments.

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Jonathan Griffin (:jgriffin)]

code comment in comment #9 fixed, and patch pushed as http://hg.mozilla.org/mozilla-central/rev/0d4182d52170