Closed Bug 703176 Opened 9 years ago Closed 9 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla11

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

The patch for bug 652494 did not completely solve the problem of uncaught JS exceptions not causing test failures in browser chrome mochitests.
Blocks: 702050
Blocks: 670831
Depends on: 703174
Assignee: nobody → cam
Status: NEW → ASSIGNED
Depends on: 702556
Depends on: 670857
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.)
Attachment #575129 - Flags: review?(jmaher)
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?
Attachment #575129 - Flags: review?(jmaher) → review+
(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.
Duplicate of this bug: 704981
Depends on: 706730
Depends on: 706734
Depends on: 706746
No longer depends on: 706730
No longer depends on: 706734
No longer depends on: 706746
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
Attachment #575129 - Attachment is obsolete: true
Attachment #578213 - Flags: review?
Attachment #578213 - Flags: review? → review?(jmaher)
Attachment #578213 - Attachment is patch: true
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?
Attachment #578213 - Flags: review?(jmaher) → review+
No, but I can add some notes to https://developer.mozilla.org/en/Mochitest for them.
https://hg.mozilla.org/mozilla-central/rev/7fe6db51869d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Backed out: https://hg.mozilla.org/mozilla-central/rev/c101c5f8c928
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 707054
https://hg.mozilla.org/mozilla-central/rev/603c426b981a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Summary: some browser chrome mochitests still not failing when uncaught JS exceptions occur → some browser chrome mochitests still not failing when uncaught JS exceptions occur, in browser chrome mochitests
Version: unspecified → Trunk
Summary: some browser chrome mochitests still not failing when uncaught JS exceptions occur, in browser chrome mochitests → some browser chrome mochitests still not failing when uncaught JS exceptions occur
Summary: some browser chrome mochitests still not failing when uncaught JS exceptions occur → Some browser chrome mochitests still not failing when uncaught JS exceptions occur. (Need to reset exception handling between tests.)
Depends on: 765200
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.