ngMock: ErrorAddingDeclarationLocationStack is no longer matched by toThrowError since jasmine 2.4.1

[jrencz]

Environment:

• ngMock 1.4.3 (I know it's old, but I can confirm the issue is not about ngMock version not being the latest)
• karma-jasmine 0.3.6
• jasmine-core 2.4.1

I try to match error thrown in my code:

angular.module('foo').constant('bar', () => {
  throw new Error('My error msg');
});

with the following test:

it('should throw expected message', () => {
  module('foo');
  inject(function (bar) {
    expect(() => {
      bar();
    }).toThrowError('My error msg');
  });
});

but I get error in the console:

Expected function to throw 'My error msg', but it threw Error: My error msg.

this is error from [email protected] toThrowError matcher.
Jasmine stopped comparing by constructor in favour of using instanceof operator in April 2015
The problem is caused by 7e916455b3

I tried modifying:

  ErrorAddingDeclarationLocationStack.prototype.toString = Error.prototype.toString;

which was ok when constructor was checked in a way that ErrorAddingDeclarationLocationStack has Error in its prototype chain:

ErrorAddingDeclarationLocationStack.prototype = Object.create(Error.prototype);
ErrorAddingDeclarationLocationStack.prototype.constructor = ErrorAddingDeclarationLocationStack;

and I can confirm it fixes the issue (error is considered an actual error again by [email protected])

@wizardwerdna can you please review this change? I'm not sure if you replaced only toString on a prototype for a reason and if it's safe to do what I suggest. I think it may not be safe to do so and this change can reverse what you achieved. In such case a better way has to be found to circumnavigate the issue you fixed in order to be able to keep using [email protected] with ngMock.

[Narretz]

ngMock has been built with the now ancient jasmine 1.3 in mind (core tests are based on 1.3). I think if we can support this change without breaking any existing tests, we can change it in ngMock, otherwise you cannot use ngMock with jasmine 2.4.1 (or this specific function)

[gkalpak]

Why don't we simply do ErrorAddingDeclarationLocationStack.prototype = Error.prototype // or new Error() ? Wouldn't that work ?

[jrencz]

@Narretz oh my ;)
That means some other unexpected things may happen.
Anyway: I'm impressed this does work with that many jasmine 2.x versions released without much intention to make it work with them.

@gkalpak That's what I think may be dangerous for some reason.
I assume what has been done and is there now is not a mistake but it has been done this way for a reason. I just can't figure out what proper prototype chain may break. And from my experience messing around with error objects may decrease angular users (i.e. developers) experience. For example Chrome started truncation message once. It took them 4 or 5 releases to track why messages got truncated without the ability to expand them on demand ;)
Browsers are very liberal in term of how to treat Error instances (all in all it's a host object) and here we're trying to add something when it's not officially supported.
That's why I'm not submitting a PR yet.

[gkalpak]

Interesting: I tried to replicate the issue with Jasmine 2.4.1, but everything seems to work as expected.
@jrencz, is there a minimal project that we can use to replicate the issue (or could you create one if there isn't one already) ?

[jrencz]

@gkalpak the problem may be that Jasmine finds no specs in your pen:

"No specs found"

I don't have experience in running jasmine out of karma therefore I don't see the reason this is like that right away. Please check if test code runs.

--- EDIT---
It does not run in Safari but it does run in Chrome.

---- EDIT2---
aha: you forgot to add babel to the equation ;)

[gkalpak]

Yeah, I wasn't using arrow functions at first, then changed to arrow functions as in your example to see if they have something to do with it. (I updated the pen.)

[Narretz]

@jrencz Can you use @gkalpak's pen and modify it so that the error becomes apparent? Babel shouldn't be that important, as long as you can provide the output that Babel produces.

[jrencz]

I tried but I wasn't able to do it.
Currently I don't have enough time to reduce my project to the bare minimum that allows to reproduce the problem.

I'm running tests using a gulp plugin on Phantom. AFAIK it shouldn't matter, but some part of this setup matters

[peabnuts123]

+1 can confirm my environment is producing exactly the same results

"angular": "1.5.0",
"angular-mocks": "^1.5.0",
"karma": "^0.13.21",
"jasmine": "^2.4.1",
"karma-jasmine": "^0.3.7"

EDIT: I have modified @gkalpak's pen to produce the error with minimal code here.
The problem is when angular itself throws the error, it causes an error of type ErrorAddingDeclarationLocationStack which does return true when jasmine checks ErrorAddingDeclarationLocationStack instanceof Error.

[gkalpak]

@peabnuts123: Thx for the reproduction.

@jrencz: I don't think there is any "danger" in changing ErrorAddingDeclarationLocationStack. After all it is our custom error; the developer didn't have a way to modify its prototype before either.
I tested your suggestion with jasmine 1.3.1 and jasmine 2.4.1 and it seems to work fine.

@jrencz or @peabnuts123: If anyone fancies submitting a PR, that would be awesome 😃

[peabnuts123]

I have put together a pull request to change the prototype of ErrorAddingDeclarationLocationStack to Error.prototype. I hope this is merged at some point so I can stop using a dodgey modified version of angular-mocks.js locally in my project =/