Closed Bug 581750 Opened 11 years ago Closed 11 years ago
"make check-one" broken
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?
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.
Option 1 has been put forward in the duplicate bug 580052.
cjones has signed off on solution #2 from comment 1, so I'll implement it.
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.
>+ msg = "TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e That line is deserving of a semicolon.
Semicolons--Bah! I always get bit by that kind of thing switching between python and JS for all this xpcshell stuff.
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+
Comment on attachment 474101 [details] [diff] [review] Review comments fixed forwarding ted's +r
Attachment #474101 - Flags: review+
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.