Closed Bug 765200 Opened 9 years ago Closed 8 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox-esr17 --- fixed
b2g18 --- fixed

People

(Reporter: emorley, Assigned: emorley)

References

Details

(Keywords: sheriffing-P3)

Attachments

(1 file, 4 obsolete files)

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;
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>
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 - ..."
Whiteboard: [sheriff-want]
WIP:
https://tbpl.mozilla.org/?tree=Try&rev=883e42ea8128
Assignee: nobody → bmo
Status: NEW → ASSIGNED
(With some deliberate removal of expectUncaughtException() to try and trigger some more error messages)
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 :-/
Attached patch WIP (obsolete) — Splinter Review
* 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)
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?
Attached patch Patch v2 (obsolete) — Splinter Review
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-
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Attached patch Patch v3 (obsolete) — Splinter Review
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+
Component: BrowserTest → Mochitest
Depends on: 803284
Whiteboard: [sheriff-want] → [sheriff-want][blocked on bug 803284]
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]
Attached patch Patch as landedSplinter Review
Attachment #668904 - Attachment is obsolete: true
Blocks: 818137
No longer depends on: 803284
https://hg.mozilla.org/mozilla-central/rev/b87db9d60554
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.