Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMock): Change ErrorAddingDeclarationLocationStack prototype #14344

Conversation

peabnuts123
Copy link
Contributor

@peabnuts123 peabnuts123 commented Mar 30, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix to angular-mocks for ErrorAddingDeclarationLocationStack error to change its prototype to Error.prototype

What is the current behavior? (You can also link to an open issue here)
Testing frameworks and other libraries interacting with angular cannot correctly detect this error with instanceof

What is the new behavior (if this is a feature change)?
This Error is correctly inherited from Error. ErrorAddingDeclarationLocationStack instanceof Error === true.

Does this PR introduce a breaking change?
No, in fact it only changes one line.

Please check if the PR fulfills these requirements

Other information:
Apologies for ridiculous branch name. I hope this was all done correctly, I am still new to git/GitHub :octocat:

change prototype to Error.prototype so test frameworks can catch it properly
#13821

change prototype to Error.prototype so test frameworks can catch it properly

angular#13821
adding test to verify change
removed trailing whitespace, fixed 4-space tab, changed to 2 spaces
@peabnuts123
Copy link
Contributor Author

Fix for issue #13821

});
}

//The following is a simplified implementation of future Jasmine's .toThrowError()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by future Jasmine's .toThrowError() ?
Why can't you use it directly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toThrowError() was introduced in Jasmine 2.0. It uses errorObject instanceof Error to check if the thrown object is an Error Object (for matching strings/regex against their .message property). Angular's build environment uses now >2-year-old Jasmine 1.3 for its own tests, so I had to rig a basic function for how Jasmine's toThrowError() fails when running this code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not any more :) We are on Jasmine 2 now 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoo! Let me amend this PR in the coming days

@Narretz
Copy link
Contributor

Narretz commented May 24, 2016

@peabnuts123 Did you ever get around to updating this PR?

@peabnuts123
Copy link
Contributor Author

Unfortunately I've been pretty busy and have been unable to update this. I still intend to, as this fix is something I need in my project. Currently just using my own fork in our build...

@peabnuts123
Copy link
Contributor Author

I've changed the unit test to utilise the real Jasmine toThrowError as mentioned in the previous comments. Apologies it took so long. All the builds work on my end, and I've confirmed everything works against master branch AND is still a necessary change.

The Travis build appears to have failed unrelated to anything I've changed? Can this be confirmed / re-run?

@gkalpak
Copy link
Member

gkalpak commented Jun 29, 2016

Travis is green now 👍

@gkalpak gkalpak closed this in f7405e3 Jun 29, 2016
gkalpak pushed a commit that referenced this pull request Jun 29, 2016
…nized as an `Error`

Change `ErrorAddingDeclarationLocationStack`'s prototype so test frameworks (such as Jasmine 2.x)
are able to recognize it as `Error`.

Fixes #13821

Closes #14344
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants