667155 - Mochitest harness no longer fails tests on unexpected exceptions

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The error hook to fail tests on unexpected exceptions that was added in bug 365929 was apparently removed in bug 652494 due to some misunderstandings about its purpose.  It needs to be restored...  In the meantime, we're missing a part of our test coverage.  :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

In particular, the onerror handler no longer fails the test but it _does_ terminate it if it's waiting for explicit finish.  So it can easily make tests start passing when they shouldn't be if they happen to throw before running most of the test.

Comment 2

[Cameron McCormack (:heycam)]

You're right.  I thought I had verified that after my logging code refactoring that uncaught exceptions still did cause test failures, but it seems not.

http://tbpl.mozilla.org/?tree=Try&rev=852a162bf44a

Here we can see a test in mochitest-plain (in mochitest-1) with an uncaught exception not causing a test failure, though it should.  The uncaught exception in mochitest-chrome does not cause a failure (deliberately, due to bug 652494).  And the one in mochitest-browser-chrome is caught, as intended.

I'll take a look in the morning.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Ensure uncaught exceptions do cause test failures in plain mochitests.

Comment 4

[Cameron McCormack (:heycam)]

Let me know if there is a nicer way of detecting whether we're in a mochitest-plain run.

With the patch applied:
http://tbpl.mozilla.org/?tree=Try&rev=7cd5bb5cead3

With the patch applied, and with a deliberate exception being thrown in one plain, one chrome and one browser chrome mochitest:
http://tbpl.mozilla.org/?tree=Try&rev=ae091ac628d9

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 541979 [details] [diff] [review]
Ensure uncaught exceptions do cause test failures in plain mochitests.

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

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +794,5 @@
> +    try {
> +        // Plain mochitests can't access Components.classes with calling
> +        // enablePrivileges first.
> +        Components.classes;
> +        isPlainMochitest = false;

I think the simplest thing to do here would just be to check window.location.protocol == "http:", since chrome tests run from chrome URLs.

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

I hate to ask this... but can we somehow add a regression test for this?  ;)

Comment 8

[Cameron McCormack (:heycam)]

If there aren't any already, we could add some mochitest sanity tests which test that failure is expected when an uncaught exception is thrown.