353010 - test_all.sh's check for a test passing isn't stringent enough

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Jeff Walden [:Waldo]]

test_all.sh checks for the success of a test by grepping for "*** PASS" in the log file.  This is insufficient to check for test success, because it's entirely possible for a test to run and indicate success but for something to happen after this which causes an assertion to occur (which is intended to be fatal for tests).

The simplest such case of this is one where an asynchronous test is improperly written such that the test indicates success *before* all callbacks from XPCOM objects occur; this usually causes the "no Components" assertion, but since that assertion occurs after "*** PASS" has been written to the file, it isn't caught by test_all.sh as an error.

Probably we just need to check the return value of the line that invokes xpcshell, but I don't know enough to say for certain that that's all that needs to happen.

Comment 1

[Dave Liebreich [:davel]]

Please attach a test case (heh) that should be reported as a failure, but is reported as PASS because of this issue.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Testcase causes assertion, doesn't cause tests to fail

Here's one that'll do the trick.

Comment 3

[Jeff Walden [:Waldo]]

make check output for this is:

../../../dist/bin/run-mozilla.sh ../../../dist/bin/test_all.sh ../../../dist/bin/necko_unit_tests
../../../dist/bin/necko_unit_tests/test_assertion.js: ../../../dist/bin/test_all.sh: line 83: 20722 Aborted                 (core dumped) DIST="$bin" $bin/xpcshell $headfiles -f $t $tailfiles 2>$t.log 1>&2
PASS

Comment 4

[Jeff Walden [:Waldo]]

(The DIST="$bin" part is from a local change I have; ignore it, because it shouldn't make a difference in demonstrating failure.)

Comment 5

[Dave Liebreich [:davel]]

Jeff, if you have the log file handy, could you please attach that to this bug as well?  

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Log file

The output's all discombobulated because there's no output buffering, but you can imagine how it was supposed to work.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch

This fixes the problem for me, as demonstrated by the current version of toolkit/mozapps/extensions/test/test_bug378216.js (passes before this patch, correctly fails after -- the assertion is bug 403180).

We need that bug to be fixed before this can be committed, because otherwise we'll break |make check| for anyone running it in a debug build.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 288885 [details] [diff] [review]
Patch

Since this is a test-only change it doesn't need approval; I'll commit this after the fix for bug 403180 is reviewed and lands.

Comment 9

[Jeff Walden [:Waldo]]

Checked in, along with tweaks to the XPCOM_DEBUG_BREAK variables in the scripts for stack-and-abort instead of just abort while I was there.