Closed Bug 521457 Opened 11 years ago Closed 11 years ago

Add debugger options to runxpcshelltests.py and runreftest.py

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(1 file, 2 obsolete files)

Add the --debugger and --debugger-args command-line options to runxpcshelltests.py in order to support running xpcshell tests under valgrind.
Attached patch patch (obsolete) — Splinter Review
This patch adds --debugger and --debugger-args to runxpcshelltests.py.  Verified locally.
Attachment #405536 - Flags: review?(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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
Duplicate of this bug: 521332
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".
Attached patch patch v3Splinter Review
Updated patch which addresses previous review comments.
Attachment #405607 - Attachment is obsolete: true
Attachment #405949 - Flags: review?(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.
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.