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

RESOLVED FIXED in mozilla11

Status

RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
The patch for bug 652494 did not completely solve the problem of uncaught JS exceptions not causing test failures in browser chrome mochitests.
(Assignee)

Updated

7 years ago
Blocks: 702050
(Assignee)

Updated

7 years ago
Blocks: 670831
(Assignee)

Updated

7 years ago
Depends on: 703174
(Assignee)

Updated

7 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Depends on: 702556
(Assignee)

Updated

7 years ago
Depends on: 670857
(Assignee)

Comment 1

7 years ago
Created attachment 575129 [details] [diff] [review]
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.)
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+
(Assignee)

Comment 3

7 years ago
(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.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 704981
(Assignee)

Updated

7 years ago
Depends on: 706730
(Assignee)

Updated

7 years ago
Depends on: 706734
(Assignee)

Updated

7 years ago
Depends on: 706746
(Assignee)

Updated

7 years ago
No longer depends on: 706730
(Assignee)

Updated

7 years ago
No longer depends on: 706734
(Assignee)

Updated

7 years ago
No longer depends on: 706746
(Assignee)

Comment 5

7 years ago
Created attachment 578213 [details] [diff] [review]
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
Attachment #575129 - Attachment is obsolete: true
Attachment #578213 - Flags: review?
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 7

7 years ago
No, but I can add some notes to https://developer.mozilla.org/en/Mochitest for them.
(Assignee)

Comment 8

7 years ago
https://hg.mozilla.org/mozilla-central/rev/7fe6db51869d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 9

7 years ago
Backed out: https://hg.mozilla.org/mozilla-central/rev/c101c5f8c928
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Depends on: 707054
https://hg.mozilla.org/mozilla-central/rev/603c426b981a
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 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.)

Updated

6 years ago
Depends on: 765200
Component: BrowserTest → Mochitest
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.