Runner#fail should work with Error-like objects

[igorraush]

Problem
The check if (!(err instanceof Error)) at https://github.com/mochajs/mocha/blob/master/lib/runner.js#L205 causes a problem when writing tests for an Angular app using the angular-mocks module. All failed assertions and/or uncaught errors from within an inject-ed function are displayed as

the object {...} was thrown, throw an Error :)

JSFiddle: http://jsfiddle.net/tybx9zwp/

Reason
The angular.mock.inject function catches errors, wraps them in an Error-like object (which does not inherit from Error), transforms the stack property, and re-throws the wrapper object. See https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L2416. The commit which introduced this mechanism is angular/angular.js@7e91645. The given justification is

Recent browsers, particularly PhantomJS 1.9.2 and Safari 7.0
treat the stack property as non-configurable and unwritable.

which seems reasonable.

Solution
This can of course be fixed quickly by replacing the instanceof check in Runner#fail with something like

if (!err || typeof err.stack !== 'string') {
  err = new Error('the ' + type(err) + ' ' + stringify(err) + ' was thrown, throw something Error-like :)');
}

but it is probably also a good idea to follow Angular's example and wrap err in an Error-like object before modifying its stack on https://github.com/mochajs/mocha/blob/master/lib/runner.js#L209.

I would be happy to submit a PR to address this, please let me know if the above approach is acceptable.

[boneskull]

sort of. comment(s) in #1678

[boneskull]

@igorraush @mochajs/mocha

Igor, thanks for the PR and bringing this to our/my attention.

As a quick bit of background, we're filtering Error.prototype.stack for more concise/useful stack traces (see mocha-clean).

PR #1678 changes the API, thus is a "breaking change". But whether or not anything will actually break is a concern:

• If we no longer emit the fail event with an Error instance (and instead an Error-like object), will any reporters break? We won't know unless we manually test this against each, because we don't have unit tests.
• Is it conceivable something else may be listening for fail and expect an Error instance? I know JetBrains has its own reporter (cc @segrey).

---

Coming at this from another angle--and I'd love some thoughts on this from others--what if we actually stopped trying to modify the stack altogether?

Error.prototype.stack is not part of any standard, and vendors can implement its API however they please (or not at all), and alter it whenever they please. The AngularJS team got bit by this. I'll argue that expecting Error.prototype.stack to work in any given way is folly.

Instead of setting Error.prototype.stack, set Error.prototype.mochaStack, which can be the filtered version. If the user has chosen to display the filtered version, the reporter should display it instead. We would get best of both worlds; we wouldn't have to stand on unstable ground, and still provide an Error instance when emitting fail--I find this much more palatable than an "Error-like object". This is more work because many reporters would have to be modified instead of runner.js. But is it a better solution?

---

If an Error-like object is thrown, then I completely agree--we should treat it as an Error. I think our major concerns here were nulls and strings being thrown; hence the friendly messaging.

@igorraush Once the dust settles and if we decide to move forward here, please squash & rebase onto branch v3.0.0 -- this is a breaking change.

[segrey]

@boneskull As for JetBrains's reporter, it doesn't expect Error instance, just an object with message and stack properties. See extractErrInfo function that is fed with err object from fail event: https://github.com/JetBrains/mocha-intellij/blob/aaa664b2ae9186123c38a38d44be007b522adb5e/lib/mochaIntellijReporter.js#L78

[igorraush]

@boneskull What do you think about separately merging the changes which actually fix this issue (i.e. modified checks in Runner#fail) and opening a new one for stack-related things?

[danielstjules]

Closing as this was fixed via #1758 and went out with 2.3.0 :)

The check is now:

if (!(err instanceof Error || err && typeof err.message == 'string')) {

Thanks!

[igorraush]

@danielstjules I've been using my fork and didn't notice :) Thanks.