551406 - xpcshell-tests - add --dumplogs to runxpcshelltests.py

Status: RESOLVED DUPLICATE of bug 581750

(Testing :: XPCShell Harness, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

Currently, runxpcshelltests.py will place test output either in separate log files or not at all but there is no option to simply dump the output to stdout. I would like to have that option so that the unittest valgrind automation can deal with xpcshell test output in a similar fashion to the other test suites.

I'm not wedded to this patch. It is simply a starting point unless you like it. I've tested it with and without --dumplogs and with and without valgrind. valgrind on a mac seems to hang at some point but that is unrelated to this I believe.

Comment 2

[(not currently active) Ted Mielczarek]

See also bug 500388.

bc: could you also make check-one pass this new argument to runxpcshelltests.py while you're at it?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)
> See also bug 500388.
> 

I still just use the output of proc.communicate so there wouldn't be incremental output the way Dave wants.

> bc: could you also make check-one pass this new argument to runxpcshelltests.py
> while you're at it?

would you want that by default as a hard-wired option to always dump the output to stdout or should I use something like EXTRA_TEST_ARGS to allow other options to be passed as well?

Comment 4

[(not currently active) Ted Mielczarek]

I'd just hardwire it in check-one. And yeah, your patch won't fix Dave's issue, it's just related.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

now with check-one goodness.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 431623 [details] [diff] [review]
patch v2

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk

FYI, you'll need to copy these changes to js/src/config/rules.mk before landing.

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>@@ -268,16 +269,27 @@ class XPCShellTests(object):
> 
>               if os.path.exists(leakLogFile):
>                 leaks = open(leakLogFile, "r")
>                 f.write(leaks.read())
>                 leaks.close()
>             finally:
>               if f:
>                 f.close()
>+
>+          if dumplogs and stdout:
>+            try:
>+              sys.stdout.write(stdout)
>+
>+              if os.path.exists(leakLogFile):
>+                leaks = open(leakLogFile, "r")
>+                sys.stdout.write(leaks.read())
>+                leaks.close()
>+            finally:
>+              pass

Looks like you can just leave the try/finally block out of this, since you're not doing anything in the finally block.

>+  parser.add_option("--no-dumplogs",
>+                    action="store_false", dest="dumplogs", default=False,
>+                    help="don't dump logs to stdout (default, only used to override --dumplogs)")

I don't think you should add --no-dumplogs. Since it's not the default, it doesn't add much value.

>+  parser.add_option("--dumplogs",
>+                    action="store_true", dest="dumplogs",
>+                    help="dump logs to stdout (default, only used to override --dumplogs)")

Fix the help text here.

r=me with those changes.

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)

this bitrotted with jmaher's landing. I'll attach a fresh patch with these changes later today.

> (From update of attachment 431623 [details] [diff] [review])
> >diff --git a/config/rules.mk b/config/rules.mk
> >--- a/config/rules.mk
> >+++ b/config/rules.mk
> 
> FYI, you'll need to copy these changes to js/src/config/rules.mk before
> landing.
> 

Oh. Thanks!

> >diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
> >--- a/testing/xpcshell/runxpcshelltests.py
> >+++ b/testing/xpcshell/runxpcshelltests.py
> >@@ -268,16 +269,27 @@ class XPCShellTests(object):

> 
> Looks like you can just leave the try/finally block out of this, since you're
> not doing anything in the finally block.

k.

> 
> >+  parser.add_option("--no-dumplogs",
> >+                    action="store_false", dest="dumplogs", default=False,
> >+                    help="don't dump logs to stdout (default, only used to override --dumplogs)")
> 
> I don't think you should add --no-dumplogs. Since it's not the default, it
> doesn't add much value.

I thought it was required when doing boolean options that the default be explicitly listed but ok.

> 
> >+  parser.add_option("--dumplogs",
> >+                    action="store_true", dest="dumplogs",
> >+                    help="dump logs to stdout (default, only used to override --dumplogs)")
> 

k.

Comment 8

[Josh Matthews [:jdm]]

What's the status here?  I'm guessing this could use a rebase + checkin-needed.

Comment 9

[Bob Clary [:bc] (inactive)]

Sorry this fell under my radar. I'll get it landed tonight.

Comment 10

[(not currently active) Ted Mielczarek]

I think cjones already landed something similar to this (I must have forgotten about this patch) in bug 566050. It adds a --verbose flag that spews the output to stdout.