Display stack for uncaught errors in the browser #3952
Conversation
|
Update: @boneskull outlines the issue & how to fix it beautifully in #2983 (comment) and this is already being done in |
|
I am a bot that watches issues for inactivity. |
|
@juergba 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. |
|
@Lindsay-Needs-Sleep I don't have the browser knowledge to review this PR. |
|
@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 will evaluate to this: 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!) |
|
I did some testing in Chrome with 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 PS: Our browser related CI tests are very weak, so they don't have much relevance. |
|
👋 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. |
|
Moving over to #5107 with a co-authored-by attribution. Cheers! 🤎 |
|
Ah @prust hello! I'd just made a new PR because I didn't have permissions to push to |
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.jswas using the pre-2014 three-parameterwindow.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 theerrparameter 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
onerrorhandler, which -- in Chrome, at least -- has an extra "Uncaught Error: " prefix. This would add a couple more lines to the top ofonerrorinbrowser-entry.js: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
beforeEach/itin 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/falsein the relevantmocha.opts, as well asmocha.allowUncaught(), but neither made a difference.Perhaps I should try putting the test with the
allowUncaughtoruncaught()tests inrunner.spec.js, but it looks like theallowUncaughttests always throw an error somewhere that it will be caught, and theuncaught()tests don't actually throw errors (they just pass an error object directly touncaught()), 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.