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

RESOLVED DUPLICATE of bug 581750

Status

Testing
XPCShell Harness
--
enhancement
RESOLVED DUPLICATE of bug 581750
8 years ago
8 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 431585 [details] [diff] [review]
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.
Attachment #431585 - Flags: review?(ted.mielczarek)
Duplicate of this bug: 551094
See also bug 500388.

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

Comment 3

8 years ago
(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?
I'd just hardwire it in check-one. And yeah, your patch won't fix Dave's issue, it's just related.
(Assignee)

Comment 5

8 years ago
Created attachment 431623 [details] [diff] [review]
patch v2

now with check-one goodness.
Attachment #431585 - Attachment is obsolete: true
Attachment #431623 - Flags: review?(ted.mielczarek)
Attachment #431585 - Flags: review?(ted.mielczarek)
Assignee: nobody → bclary
Attachment #431623 - Flags: review?(ted.mielczarek) → review+
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.
(Assignee)

Comment 7

8 years ago
(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

8 years ago
What's the status here?  I'm guessing this could use a rebase + checkin-needed.
(Assignee)

Comment 9

8 years ago
Sorry this fell under my radar. I'll get it landed tonight.
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.
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 581750
You need to log in before you can comment on or make changes to this bug.