670817 - Test that uncaught exceptions cause mochitest failures

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Cameron McCormack (:heycam)]

Per bug 667155 comment 10, we should add a sanity test to ensure uncaught exceptions in mochitests cause failures.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Check that uncaught exceptions cause test failures.

This adds sanity check tests for plain, chrome and browser-chrome mochitests, checking that an uncaught exception would otherwise have resulted in a test failure being reported.  The chrome and browser-chrome ones are starting off disabled; I'll file a bug blocking bug 652494 to enable them.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 545315 [details] [diff] [review]
Check that uncaught exceptions cause test failures.

Review of attachment 545315 [details] [diff] [review]:
-----------------------------------------------------------------

Are there specific tests that are trying to throw exceptions?  This seems similar to expectChildProcessCrash() except for the main process.  It seems that we are making things more complicated with yet another state variable, but I can't think of another solution.  One thing I would like to make sure is covered is the scenario where we have a open('window_test.html', ...) and in window_test.html we specifically include SimpleTest.js.  Will these state variables work in that situation?

::: testing/mochitest/chrome/Makefile.in
@@ +48,5 @@
>  		test_synthesizeDragStart.xul \
>  		test_synthesizeDrop.xul \
> +# Disabled until bug 652494 is resolved.
> +#		test_sanityException.xul \
> +#		test_sanityException2.xul \

these files are not included in the patch, please include them or remove this comment.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +819,1 @@
>      }

can we make this use logResult (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#115).  This would mean we could get these errors on Android devices as well because we don't have stdout there, only explicit file logging.

@@ +825,5 @@
>              // Ignore return value: always run default handler.
>              gOldOnError(errorMsg, url, lineNumber);
>          } catch (e) {
>              // Log the error.
> +            dump("Exception thrown by gOldOnError(): " + e + "\n");

ditto on the logResult

::: testing/mochitest/tests/browser/Makefile.in
@@ +53,5 @@
>  	                  browser_popupNode.js \
>  	                  browser_popupNode_check.js \
> +# Disabled until bug 652494 is resolved.
> +#			  browser_sanityException.js \
> +#			  browser_sanityException2.js \

these files are not included in the patch, please include them or remove this comment.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to comment #2)
> Are there specific tests that are trying to throw exceptions?

There are the sanity tests that I want to add here that would use this.  I think the tests mentioned in bug 670857 might also benefit from it, since those tests are checking that the web console notices uncaught exceptions.

> This seems
> similar to expectChildProcessCrash() except for the main process.  It seems
> that we are making things more complicated with yet another state variable,
> but I can't think of another solution.  One thing I would like to make sure
> is covered is the scenario where we have a open('window_test.html', ...) and
> in window_test.html we specifically include SimpleTest.js.  Will these state
> variables work in that situation?

In bug 669251 I'm going to make SimpleTest.js loaded in secondary windows delegate everything to the main window's SimpleTest object, to avoid problems like this.

> ::: testing/mochitest/chrome/Makefile.in
> ...
> these files are not included in the patch, please include them or remove
> this comment.

Ack (meant to include them).

> ::: testing/mochitest/tests/SimpleTest/SimpleTest.js
> @@ +819,1 @@
> >      }
> 
> can we make this use logResult
> (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/SimpleTest.js#115).  This would mean we could get these errors on
> Android devices as well because we don't have stdout there, only explicit
> file logging.

Sure, I'll use _logInfo (which calls _logResult).

> @@ +825,5 @@
> >              // Ignore return value: always run default handler.
> >              gOldOnError(errorMsg, url, lineNumber);
> >          } catch (e) {
> >              // Log the error.
> > +            dump("Exception thrown by gOldOnError(): " + e + "\n");
> 
> ditto on the logResult

Ack.

> ::: testing/mochitest/tests/browser/Makefile.in
> @@ +53,5 @@
> >  	                  browser_popupNode.js \
> >  	                  browser_popupNode_check.js \
> > +# Disabled until bug 652494 is resolved.
> > +#			  browser_sanityException.js \
> > +#			  browser_sanityException2.js \
> 
> these files are not included in the patch, please include them or remove
> this comment.

Ack.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Check that uncaught exceptions cause test failures. (v2)

Address review comments.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 545749 [details] [diff] [review]
Check that uncaught exceptions cause test failures. (v2)

Review of attachment 545749 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the dump statements adddressed.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +831,3 @@
>              // Log its stack.
>              if (e.stack) {
> +                dump("JavaScript error stack:\n" + e.stack + "\n");

should these two dump() statements be SimpleTest.info() calls instead?

Comment 6

[Cameron McCormack (:heycam)]

Yes, will fix before landing.  (I'm assuming it doesn't matter that the logging mechanism is used with multiline strings.)

Comment 7

[Cameron McCormack (:heycam)]

(Also the "\n"s at the end need to be dropped, including for the line I already changed to use SimpleTest.info().)

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

it shouldn't matter as we have plenty of multiline output currently.  A try server run will always be a good litmus test of the buildbot toolchain!

Comment 9

[Cameron McCormack (:heycam)]

Try run verified multiline output was reported from Android builds correctly.  Landed on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6d85a68e820e

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6d85a68e820e