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

RESOLVED FIXED in mozilla1.9


13 years ago
10 years ago


(Reporter: Waldo, Assigned: Waldo)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(3 attachments)



13 years ago
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.
Please attach a test case (heh) that should be reported as a failure, but is reported as PASS because of this issue.

Comment 2

13 years ago
Created attachment 238857 [details]
Testcase causes assertion, doesn't cause tests to fail

Here's one that'll do the trick.

Comment 3

13 years ago
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

Comment 4

13 years ago
(The DIST="$bin" part is from a local change I have; ignore it, because it shouldn't make a difference in demonstrating failure.)
Jeff, if you have the log file handy, could you please attach that to this bug as well?  

Comment 6

13 years ago
Created attachment 238860 [details]
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

11 years ago
Created attachment 288885 [details] [diff] [review]

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.
Assignee: nobody → jwalden+bmo
Attachment #288885 - Flags: review?


11 years ago
Attachment #288885 - Flags: review? → review?(rcampbell)


11 years ago
Depends on: 403180
Attachment #288885 - Flags: review?(rcampbell) → review+

Comment 8

11 years ago
Comment on attachment 288885 [details] [diff] [review]

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

11 years ago
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.
Last Resolved: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Depends on: 412216
No longer depends on: 412216
Component: Testing → TUnit
Product: Core → Testing
QA Contact: testing → tunit
Target Milestone: mozilla1.9beta2 → mozilla1.9
You need to log in before you can comment on or make changes to this bug.