Closed Bug 581750 Opened 11 years ago Closed 11 years ago

"make check-one" broken


(Testing :: XPCShell Harness, defect)

Windows CE
Not set


(Not tracked)



(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)




(1 file, 3 obsolete files)

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

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?
Attached patch fixes printf of error under e10s (obsolete) — Splinter Review
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.
Attachment #460096 - Attachment is patch: true
Attachment #460096 - Attachment mime type: application/octet-stream → text/plain
Option 1 has been put forward in the duplicate bug 580052.
Depends on: 580052
OS: Linux → Windows CE
Duplicate of this bug: 581835
Duplicate of this bug: 580052
cjones has signed off on solution #2 from comment 1, so I'll implement it.
Assignee: nobody → jduell.mcbugs
Duplicate of this bug: 551406
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)
>+      msg = "TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e

That line is deserving of a semicolon.
Attached patch Add semicolon to JS file (obsolete) — Splinter Review
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)
Giving snazzy new title in case that speeds review up :)
Summary: --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/ b/testing/xpcshell/
>--- a/testing/xpcshell/
>+++ b/testing/xpcshell/
>@@ -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+
Attachment #460707 - Attachment is obsolete: true
Comment on attachment 474101 [details] [diff] [review]
Review comments fixed

forwarding ted's +r
Attachment #474101 - Flags: review+
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.