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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Ed Morley [:emorley]]

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;

Comment 1

[Ed Morley [:emorley]]

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>

Comment 2

[Ed Morley [:emorley]]

WIP:
https://tbpl.mozilla.org/?tree=Try&rev=883e42ea8128

Comment 3

[Ed Morley [:emorley]]

(With some deliberate removal of expectUncaughtException() to try and trigger some more error messages)

Comment 4

[Ed Morley [:emorley]]

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 :-/

Comment 5

[Ed Morley [:emorley]]

Attachment: 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)

Comment 6

[Ed Morley [:emorley]]

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?

Comment 7

[Ed Morley [:emorley]]

Attachment: 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)

Comment 8

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

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.

Comment 9

[Ed Morley [:emorley]]

Attachment: 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.

Comment 10

[Ed Morley [:emorley]]

Attachment: Patch v3

qref output an empty patch previously (does this every now and again :-/)

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Ed Morley [:emorley]]

Attachment: Patch as landed

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b87db9d60554

Comment 14

[Ed Morley [:emorley]]

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