If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Status

Testing
XPCShell Harness
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

We added the --verbose flag in bug 566050 so we could see stdout/stderr from tests (which previously was only shown if test failed IIRC).  We did this by passing stdout=null to Popen when --verbose, so we no longer pipe the output to runxpcshell.py.

Alas, we detect success/failure by grepping through the piped output for TEST-UNEXPECTED-FAILURE.  So now passing --verbose means we always print "passed" even if a test failed.  This doesn't break tinderbox, etc. runs, since --verbose isn't on by default for "make xpcshell-tests", but it is on for "check-one", which is confusing.

Two solutions come to mind:

1) Don't turn on --verbose by default for check-one, and when --verbose is on, don't print success or fail (maybe print something saying we don't check?). Easy, but not a good fix

2) Capture stdout/stderr in pipes even for --verbose, and print them out when the test is done.  One wrinkle:  we took out the SIGINT handler that I added a while ago (which allowed runxpcshell to survive control-C, but kill a hung test, so we can see whatever stdout/err the process had printed up to the point we killed it), and we'd need to add that back if we want to see whatever stdout/err the process had printed up to the point we killed it.

I forget if cjones had a good reason why he wanted the SIGINT removed.  Probably he wanted to be able to stop a full run of xpcshell tests? (it was irritating to no longer be able to kill runxpcshell except by kill -9).  If that's the only reason, we could install a SIGINT handler that notes that control-c was called and sets a flag that causes the mainline code to print the output from the killed test, and then exit w/o running more tests.   Although there have been times when I've wanted to kill a hung test but let the rest of the tests continue, so maybe we could have a --keep-going flag for that?
(Assignee)

Comment 1

7 years ago
Created attachment 460096 [details] [diff] [review]
fixes printf of error under e10s

Perhaps we can shoehorn in this patch to our fix for this bug.  Right now the line that prints TEST-UNEXPECTED-FAIL errors uses a separate _dump command for the "\n" when there's no stack trace (there never seems to be a stack trace, BTW).  This causes an extra, confusing "child: " to be printed at the end of the line under e10s.
(Assignee)

Updated

7 years ago
Attachment #460096 - Attachment is patch: true
Attachment #460096 - Attachment mime type: application/octet-stream → text/plain

Comment 2

7 years ago
Option 1 has been put forward in the duplicate bug 580052.

Updated

7 years ago
Depends on: 580052

Updated

7 years ago
OS: Linux → Windows CE

Updated

7 years ago
Duplicate of this bug: 581835
(Assignee)

Updated

7 years ago
Duplicate of this bug: 580052
(Assignee)

Comment 5

7 years ago
cjones has signed off on solution #2 from comment 1, so I'll implement it.
(Assignee)

Updated

7 years ago
Assignee: nobody → jduell.mcbugs

Updated

7 years ago
Duplicate of this bug: 551406
Depends on: 566050
(Assignee)

Comment 7

7 years ago
Created attachment 460695 [details] [diff] [review]
Fix --verbose as per solution #2; also includes head.js printf fix.

OK, I've tested this patch manually for the various permutations of make check-one and make xpcshell with/without the new --keep-going flag, with success/fail/hung tests, and it works as spec'd.

Remind me to update docs on MDC if you +r.
Attachment #460096 - Attachment is obsolete: true
Attachment #460695 - Flags: review?(ted.mielczarek)

Comment 8

7 years ago
>+      msg = "TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e

That line is deserving of a semicolon.
(Assignee)

Comment 9

7 years ago
Created attachment 460707 [details] [diff] [review]
Add semicolon to JS file

Semicolons--Bah!  I always get bit by that kind of thing switching between python and JS for all this xpcshell stuff.
Attachment #460695 - Attachment is obsolete: true
Attachment #460707 - Flags: review?(ted.mielczarek)
Attachment #460695 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 10

7 years ago
Giving snazzy new title in case that speeds review up :)
Summary: runxpcshell.py --verbose option breaks ability to determine if xpcshell test failed → "make check-one" broken
Comment on attachment 460707 [details] [diff] [review]
Add semicolon to JS file

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>@@ -41,16 +41,22 @@
> import re, sys, os, os.path, logging, shutil, signal, math
> from glob import glob
> from optparse import OptionParser
> from subprocess import Popen, PIPE, STDOUT
> from tempfile import mkdtemp, gettempdir
> 
> from automationutils import *
> 
>+""" Control-C handling """
>+gotSIGINT = 0
>+def markGotSIGINT(signum, stackFrame):
>+  global gotSIGINT 
>+  gotSIGINT = 1
>+

Use True / False for the values here.

>+        if (gotSIGINT):

Parentheses not needed.

>+          print "ERROR | Received SIGINT (control-C) during test execution"

"ERROR" isn't something we usually print. You can print INFO or TEST-UNEXPECTED-FAIL if you want to treat it as fatal.

>+    if (gotSIGINT and not keepGoing):

No parens.

>+      print "ERROR | Received SIGINT (control-C), so stopped run. " \
>+            + "(Use --keep-going to keep running tests after killing one with SIGINT)"

You don't need the + after the continuation, Python will concat the strings for you.

r=me with those fixes.
Attachment #460707 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 12

7 years ago
Created attachment 474101 [details] [diff] [review]
Review comments fixed
Attachment #460707 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Comment on attachment 474101 [details] [diff] [review]
Review comments fixed

forwarding ted's +r
Attachment #474101 - Flags: review+
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d8203cb80c95
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.