Display stack for uncaught errors in the browser

[prust]

Description of the Change

This change captures the stack of uncaught exceptions in the browser, so it will be displayed in Mocha. Before this change, uncaught exceptions in the browser are displayed with a URL and line number, but without a stack:

This pull request updates the global error handler for the browser so that the stack is captured:

The issue was that the browser-specific hook for uncaughtExceptions in browser-entry.js was using the pre-2014 three-parameter window.onerror, which only provides the error message, the URL and the line number. In 2014, two more parameters were added in both the spec and Chrome, with other browsers following shortly thereafter (including IE11) -- the last of these parameters is the Error object itself, which has a populated stack.

Alternate Designs

The design could be simplified by only supporting the five-parameter onerror(), but since mocha is used in all sorts of environments, I figured it was safest to use the err parameter if it exists and to fall back to the previous functionality if it doesn't.

The design could be a little more complex if it is important to preserve the message provided to the onerror handler, which -- in Chrome, at least -- has an extra "Uncaught Error: " prefix. This would add a couple more lines to the top of onerror in browser-entry.js:

if (err)
  err.message = msg;

Why should this be in core?

This would bring consistency to the quality of error reporting in mocha's core (uncaught exceptions in Node and caught exceptions in Node and the Browser all include stacks).

Benefits

The inclusion of the stack will make it easier for developers to determine the cause of the error.

Possible Drawbacks

With this particular design choice, if a mocha user was grepping or testing for the browser-supplied "Uncaught Error:" prefix in a browser test, this change would cause it to not be found. If this is important, the two lines mentioned above in "Alternate Designs" could be added.

Applicable issues

• I think Mocha 2.3.4: Errors That Occur Outside of beforeEach/it in an AMD Module Lose Stack Trace #2167 may be fixed by this pull request (but I'm not familiar with AMD/RequireJS, so I'm not in a good position to test it)

Regarding semver, I think I would call this an enhancement (providing stack traces for uncaught exceptions). I suppose it could be considered a breaking change, since it is hypothetically possible that someone is relying on the browser-supplied "Uncaught Error:" prefix somehow, but this is highly unlikely. If it's a concern, it could be address by the 2-line fix above.

Tests

I did write a browser-specific test for this (which was orders of magnitude harder than fixing the issue itself), but the uncaught exception is causing the test to fail. I tried passing --allow-uncaught true/false in the relevant mocha.opts, as well as mocha.allowUncaught(), but neither made a difference.

Perhaps I should try putting the test with the allowUncaught or uncaught() tests in runner.spec.js, but it looks like the allowUncaught tests always throw an error somewhere that it will be caught, and the uncaught() tests don't actually throw errors (they just pass an error object directly to uncaught()), the former would probably run into the same issue I'm currently running into and the latter wouldn't exercise the code in question.

If someone with familiarity with the mocha's exception-testing tests could lend a hand or direct me in a direction that is likely to work, I would appreciate it.

Fixes #5106.

[coveralls]

Coverage increased (+0.07%) to 92.925% when pulling 56e2232 on CSNW:master into ac12f2c on mochajs:master.

[prust]

Update: @boneskull outlines the issue & how to fix it beautifully in #2983 (comment) and this is already being done in throw.spec.js. I'm going to follow suit here (and/or move this test into throw.spec.js)

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[Lindsay-Needs-Sleep]

@juergba
Can you review this issue?

I have been using this improvement. It is very useful. I would like to see it in the next release.

I have also run ‘npm run test’ on this branch, everything passes.

[juergba]

@Lindsay-Needs-Sleep I don't have the browser knowledge to review this PR.
Mocha is still supporting IE11, this: Browser compatibility doesn't help neither.

[Lindsay-Needs-Sleep]

@juergba Do you know who might have the knowledge (and power to review this?)

Due to the way javascript works, if a browser does not support the 4th and 5th arguments of onerror they will just have a value of undefined when onerror is triggered. This is fine because in that situation, this line:

fn(err || new Error(msg + ' (' + url + ':' + line + ')'));

will evaluate to this:

fn(new Error(msg + ' (' + url + ':' + line + ')'));

Which is exactly what happened before this change.

Basically, this change just results in an improved error object for browsers that do support the 4th and 5th args. Browsers that don't, will be unaffected.

(Also, I did try out IE11. It does support the 4th and 5th args! Yay!)

[outsideris]

I also confirmed IE11 supports onerror with 5 arguments.

[juergba]

@prust Are you still interested in this PR?
I'm trying to understand and finally get it merged.

If yes, could you please rebase? I would like to push your branch to our repo, to make sure all tests do pass correctly. There are additional tests running (eg IE11).

Does this PR solve #1979 ?

[juergba]

I did some testing in Chrome with fullTrace: true and reporter: 'spec':

describe('uncaught', function() {
  it('throw delayed error', (done) => {
    setTimeout(() => {
      throw new Error('Whoops!');
    }, 10)
    setTimeout(done, 10);
  });

Now there is even less stack information printed than before. I guess skipping the msg is not a good idea. An uncaughtException does not always mean that the runner throws and aborts. In above example the handler recovers from the error (fails the still open test) and then keeps going. In this case we loose stack information now.
I will do more testing tomorrow.

PS: Our browser related CI tests are very weak, so they don't have much relevance.

[JoshuaKGoldberg]

👋 coming back to this @prust, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

I do see that it was previously approved, so I'm betting either way we can review this soon and hopefully get it merged with minimal changes.

[JoshuaKGoldberg]

Moving over to #5107 with a co-authored-by attribution. Cheers! 🤎

[JoshuaKGoldberg]

Ah @prust hello! I'd just made a new PR because I didn't have permissions to push to CSNW:master. If you want to make a new PR from another branch I'm happy to switch to that one, to preserve attribution for you as the author. No worries if you don't have the time though. 🙂

[JoshuaKGoldberg]

#5107 is merged; we'll release a new version with the fix soon. 🚀

[prust]

Thanks @JoshuaKGoldberg!