Shorten errors of type: "an unexpected uncaught JS exception reported through window.onerror - ..."

RESOLVED FIXED in Firefox 18

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

({sheriffing-P3})

Trunk
mozilla20
sheriffing-P3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed, firefox-esr17 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Whenever I file a bug for a new intermittent failure of the form:

> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_632817.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: content.wrappedJSObject.testXhrGet is not a function at chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_632817.js:79
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_632817.js | Test timed out

I frequently have to hand edit the string before putting it into the bug summary, since it exceeds the max summary length (particularly if there were more errors than just the example two above).

I would love it if we could shorten:
"an unexpected uncaught JS exception reported through window.onerror"

To something like:
"uncaught exception"

Or if we really need to mention the expected vs unexpected still (I presume the 'expected' are todos?):
"uncaught exception" for unexpected and "TODO uncaught exception" for expected

At:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#991

991     var message = "an " + (isExpected ? "" : "un") + "expected uncaught JS exception reported through window.onerror";
992     var error = errorMsg + " at " + url + ":" + lineNumber;
(Assignee)

Comment 1

6 years ago
Oh and if the exception occurred in the same file for which:
TEST-UNEXPECTED-FAIL | chrome://<file>

then we could just show:
TEST-UNEXPECTED-FAIL | chrome://<file> | uncaught exception - <error> at <line-number>
and not:
... at <file>:<linenumber>
(Assignee)

Updated

6 years ago
Summary: Shorten errors of type: "an unexpected uncaught JS exception reported through window.onerror - TypeError: foo.bar is not a function at chrome://mochitests/content/browser/browser/devtools/webconsole/test/foo.js:N" → Shorten errors of type: "an unexpected uncaught JS exception reported through window.onerror - ..."
(Assignee)

Updated

6 years ago
Whiteboard: [sheriff-want]
(Assignee)

Comment 2

6 years ago
WIP:
https://tbpl.mozilla.org/?tree=Try&rev=883e42ea8128
Assignee: nobody → bmo
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
(With some deliberate removal of expectUncaughtException() to try and trigger some more error messages)
(Assignee)

Comment 4

6 years ago
Ok, so there are a few varients of |url| which causes SimpleTest._getCurrentTestURL() != url a lot of the time; meaning we miss out on the shortening

Examples:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_464620_b.js | uncaught exception - ReferenceError: setup is not defined at http://mochi.test:8888/browser/browser/components/sessionstore/test/browser_464620_b.html:1

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.js | uncaught exception - TypeError: window.foobarBug762996click is not a function at http://example.com/browser/browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.html:25

I've tried seeing if there was an existing way in SimpleTest.js to get the test chrome:// path to match against, rather than the URL, but couldn't see anything obvious from my skim though.

Guess we could just do some string manipulation, but seems a bit unnecessary/messy :-/
(Assignee)

Comment 5

6 years ago
Created attachment 647931 [details] [diff] [review]
WIP

* Shortens "an {un}expected uncaught JS exception reported through window.onerror" to "{expected} uncaught exception"
* Replaces the full test URL with just the line number, if the exception occurred in the test itself (though misses the cases mentioned above.
* Remove the unused href variable (made redundant by bug 652494 / e804728b128b)
(Assignee)

Comment 6

6 years ago
Joel, in this bug I'm trying to replace failures of the form:
> TEST-UNEXPECTED-FAIL | chrome://<long-path>/foo.js | uncaught JS exception - <exception> at chrome://<long-path>/foo.js:NN
> TEST-UNEXPECTED-FAIL | chrome://<long-path>/foo.js | uncaught JS exception - <exception> at http://mochi.test:8888/<long-path>/foo.js:NN
> TEST-UNEXPECTED-FAIL | chrome://<long-path>/foo.js | uncaught JS exception - <exception> at http://example.com/<long-path>/foo.js:NN
with:
> TEST-UNEXPECTED-FAIL | chrome://<long-path>/foo.js | uncaught JS exception - <exception> at line NN

(ie: if the exception occurred in the test being run, don't repeat the location and thus unnecessarily bloat the failure that shows on TBPL)

My WIP patch only currently covers the first case above; since for the others |SimpleTest._getCurrentTestURL() != url_of_exception|

Can you think of an easier way to cover the remaining cases without just chopping off the path and matching only on filename?
(Assignee)

Comment 7

6 years ago
Created attachment 668898 [details] [diff] [review]
Patch v2

Failing finding a better way to get matching full paths (per comment 6), this just compares the filename.

* Shortens "an {un}expected uncaught JS exception reported through window.onerror" to "{expected} uncaught exception"
* Replaces the full test URL with just the line number, if the exception occurred in a filename that matches the test filename.
* Removes the unused href variable (made redundant by bug 652494 / e804728b128b)
Attachment #647931 - Attachment is obsolete: true
Attachment #668898 - Flags: review?(jmaher)
Comment on attachment 668898 [details] [diff] [review]
Patch v2

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

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +275,5 @@
>  
> +SimpleTest._getFilenameFromPath = function(path) {
> +    var filenameIndex = path.lastIndexOf("/") + 1;
> +    return path.slice(filenameIndex);
> +};

there is no error checking here.  What if path is null or there is no +1 option.
Attachment #668898 - Flags: review?(jmaher) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 668902 [details] [diff] [review]
Patch v3

With checking for string in _getFilenameFromPath().

Note: Intentionally not checking .lastIndexOf() != -1, since I think it would be useful to have this working for passing in something that is already a filename. This also means that when used with _getCurrentTestURL(), we could get "unknown test url" returned from _getFilenameFromPath(), but I think that's still accurate and fine for this.
Attachment #668898 - Attachment is obsolete: true
Attachment #668902 - Flags: review?(jmaher)
(Assignee)

Comment 10

6 years ago
Created attachment 668904 [details] [diff] [review]
Patch v3

qref output an empty patch previously (does this every now and again :-/)
Attachment #668902 - Attachment is obsolete: true
Attachment #668902 - Flags: review?(jmaher)
Attachment #668904 - Flags: review?(jmaher)
Attachment #668904 - Flags: review?(jmaher) → review+
(Assignee)

Updated

6 years ago
Component: BrowserTest → Mochitest
(Assignee)

Updated

6 years ago
Depends on: 803284
(Assignee)

Updated

6 years ago
Whiteboard: [sheriff-want] → [sheriff-want][blocked on bug 803284]
(Assignee)

Comment 11

6 years ago
I've landed the shorten string part of this here, so we can get it in the tree and will break out the "hide filepath of exception location, if same as currently running test" part to another bug.

http://hg.mozilla.org/integration/mozilla-inbound/rev/b87db9d60554
Whiteboard: [sheriff-want][blocked on bug 803284] → [sheriff-want]
(Assignee)

Comment 12

6 years ago
Created attachment 688320 [details] [diff] [review]
Patch as landed
Attachment #668904 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 818137
(Assignee)

Updated

6 years ago
No longer depends on: 803284
https://hg.mozilla.org/mozilla-central/rev/b87db9d60554
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d4b7c2b4c9c
https://hg.mozilla.org/releases/mozilla-beta/rev/0483a84f38d1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9d8d6a24b044
https://hg.mozilla.org/releases/mozilla-esr17/rev/55fb16d078e6
status-b2g18: --- → fixed
status-firefox18: --- → fixed
status-firefox19: --- → fixed
status-firefox-esr17: --- → fixed
Keywords: sheriffing-P3
Whiteboard: [sheriff-want]
You need to log in before you can comment on or make changes to this bug.