581750 - "make check-one" broken

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Jason Duell]

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?

Comment 1

[Jason Duell]

Attachment: 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.

Comment 2

[Josh Matthews [:jdm]]

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

Comment 5

[Jason Duell]

cjones has signed off on solution #2 from comment 1, so I'll implement it.

Comment 7

[Jason Duell]

Attachment: 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.

Comment 8

[Josh Matthews [:jdm]]

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

That line is deserving of a semicolon.

Comment 9

[Jason Duell]

Attachment: 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.

Comment 10

[Jason Duell]

Giving snazzy new title in case that speeds review up :)

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Jason Duell]

Attachment: Review comments fixed

Comment 13

[Jason Duell]

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

forwarding ted's +r

Comment 14

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/d8203cb80c95