Closed Bug 670817 Opened 8 years ago Closed 8 years ago

Test that uncaught exceptions cause mochitest failures

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla8

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file, 1 obsolete file)

Per bug 667155 comment 10, we should add a sanity test to ensure uncaught exceptions in mochitests cause 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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #545315 - Flags: review?
Attachment #545315 - Flags: review? → review?(jmaher)
Blocks: 670831
Blocks: 670857
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.
Attachment #545315 - Flags: review?(jmaher) → review-
(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.
Address review comments.
Attachment #545315 - Attachment is obsolete: true
Attachment #545749 - Flags: review?(jmaher)
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?
Attachment #545749 - Flags: review?(jmaher) → review+
Yes, will fix before landing.  (I'm assuming it doesn't matter that the logging mechanism is used with multiline strings.)
(Also the "\n"s at the end need to be dropped, including for the line I already changed to use SimpleTest.info().)
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!
Try run verified multiline output was reported from Android builds correctly.  Landed on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6d85a68e820e
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/6d85a68e820e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.