703176 - Some browser chrome mochitests still not failing when uncaught JS exceptions occur. (Need to reset exception handling between tests.)

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Cameron McCormack (:heycam)]

The patch for bug 652494 did not completely solve the problem of uncaught JS exceptions not causing test failures in browser chrome mochitests.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Ensure all browser chrome mochitests do fail when uncaught JS exceptions occur.

The main problem here was that due to browser-test.js reusing the one
SimpleTest object for the entire browser chrome mochitest run, we
should have been resetting SimpleTest._expectingUncaughtException
and SimpleTest._ignoringAllUncaughtExceptions.  This patch does this,
as well as correctly handling uncaught exceptions in these cases when
we want to ignore them.

The changes in testScope I made since during a non-buggy test run,
aTest should always be the same as this.__browserTest.  (It isn't
the case for a buggy test run, where for example a single test
calls finish() more than once, but in that case you probably want
to report the original test name rather than whatever the current
test name is.)

I cleaned up the message text in SimpleTest's window.onerror to make
it a bit nicer to read in logs.

As a result of this, a number of additional tests needed
ignoreAllUncaughtExceptions() calls to mask the latent bugs in them.

(This patch should only be landed once the dependent bugs are fixed,
since the patches in those bugs fix a few remaining uncaught exceptions
that were either simple to fix or could not be hidden with an
ignoreAllUncaughtExceptions() call.)

Comment 2

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

Comment on attachment 575129 [details] [diff] [review]
Ensure all browser chrome mochitests do fail when uncaught JS exceptions occur.

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

I don't really like the explicit addition of ignoreAllUncaughtExceptions() in browser_.js files.  Why not add it to all files?

Do these tests pass on try server for android and all desktop platforms?

::: testing/mochitest/browser-test.js
@@ +263,5 @@
>      this.currentTest.scope.SimpleTest = this.SimpleTest;
>      this.currentTest.scope.gTestPath = this.currentTest.path;
>  
>      // Override SimpleTest methods with ours.
> +    ["ok", "is", "isnot", "todo", "todo_is", "todo_isnot", "info"].forEach(function(m) {

good catch!

@@ +393,5 @@
>  
>    var self = this;
>    this.ok = function test_ok(condition, name, diag, stack) {
> +    aTest.addResult(new testResult(condition, name, diag, false,
> +                                   stack ? stack : Components.stack.caller));

do we need self.__browserTest for anything else?

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +757,5 @@
>      });
>  }
>  
> +SimpleTest.reset();
> +

why is this here and not in .finish() function?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Joel Maher (:jmaher) from comment #2)
> I don't really like the explicit addition of ignoreAllUncaughtExceptions()
> in browser_.js files.  Why not add it to all files?

I only added them to the files that currently still do have (unintended) uncaught JS exceptions but which are not triggering test failures.  I have filed bugs for all of these tests to fix the underlying issue, and they are all blocking bug 702050.

> Do these tests pass on try server for android and all desktop platforms?

Yes: https://tbpl.mozilla.org/?tree=Try&rev=e7ed7eb50b1b

> @@ +393,5 @@
> >  
> >    var self = this;
> >    this.ok = function test_ok(condition, name, diag, stack) {
> > +    aTest.addResult(new testResult(condition, name, diag, false,
> > +                                   stack ? stack : Components.stack.caller));
> 
> do we need self.__browserTest for anything else?

No, there are no other uses of it.

> ::: testing/mochitest/tests/SimpleTest/SimpleTest.js
> @@ +757,5 @@
> >      });
> >  }
> >  
> > +SimpleTest.reset();
> > +
> 
> why is this here and not in .finish() function?

It's not in SimpleTest's finish() because the resetting only needs to happen when browser-test.js is using SimpleTest.  But the reason I had it here is that I wanted to initialize those two properties because browser-test.js will call isIgnoringAllUncaughtExceptions() before the property exists, causing a JS warning to be shown about accessing a non-existent property.

However it is not strictly needed for non-browserc-chrome mochitests, so I might move it to somewhere in browser-test.js just before any tests run.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Ensure all browser chrome mochitests do fail when uncaught JS exceptions occur. (v1.1)

I moved the SimpleTest.reset() call to browser-test.js and had to add a couple more ignoreAllUncaughtExceptions() calls to handle recently added tests as well as for some tests whose fixes aren't progressing quickly, so thought I'd re-request review.

Try run green except for random oranges: https://tbpl.mozilla.org/?tree=Try&rev=77b91be6661b

Comment 6

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

Comment on attachment 578213 [details] [diff] [review]
Ensure all browser chrome mochitests do fail when uncaught JS exceptions occur. (v1.1)

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

this looks clean.  Have we updated docs for writing mochitests to outline how these expect/ignore exceptions work?

Comment 7

[Cameron McCormack (:heycam)]

No, but I can add some notes to https://developer.mozilla.org/en/Mochitest for them.

Comment 8

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/mozilla-central/rev/7fe6db51869d

Comment 9

[Cameron McCormack (:heycam)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/c101c5f8c928

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/603c426b981a

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/603c426b981a