Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for passing a function to stub.throws(...) #1511

Merged
merged 1 commit into from Aug 6, 2017

Conversation

noisecapella
Copy link
Contributor

Purpose (TL;DR) - mandatory

Fixes #1507
This PR adds functionality to allow stub.throws(...) to accept a function as an argument. This function is invoked at the point that the exception is thrown which allows the end user to have a more descriptive stack trace for the error. This also delays instantiating the Error until it is thrown (except when the user has already instantiated it) to create a more detailed stack trace.

Background (Problem in detail) - optional

At least in node, the stack trace for an Error is created when the object is instantiated rather than when it's thrown (from here). The stack trace is sometimes useful in figuring out what pieces of code are involved with an error. Currently errors used in throws(...) are instantiated by the caller or soon after in the setup of the stub which results in a stack trace showing where the setup occurred rather than where the problem occurred.

Solution - optional

In cases where the exception is not passed into throws(...), a function which will instantiate the exception is used internally instead and assigned to a new field exceptionCreator. When the exception is thrown exceptionCreator is instantiated and the result is assigned to exception to allow inspection via toString or other means.

How to verify - mandatory

To see the difference in stack trace see the test I included in the linked issue.

@fatso83
Copy link
Contributor

fatso83 commented Aug 3, 2017

Looks good, but need to test it to get a feel for it :-)

@mantoni
Copy link
Member

mantoni commented Aug 4, 2017

Nice work! I'm missing test cases verifying that the error object is now created lazily for all three cases.

@mantoni mantoni added Improvement semver:minor changes will cause a new minor version labels Aug 4, 2017
@noisecapella
Copy link
Contributor Author

I added some tests, are those assertions what you were thinking of?

@mantoni
Copy link
Member

mantoni commented Aug 4, 2017

In general, yes. However, you're now testing the implementation details, not the behavior. I was thinking of asserting the point in time a given function is actually called, or stubbing the Error constructor and checking the point in time when it's created.

@noisecapella
Copy link
Contributor Author

I see, I'll make the change

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

Think those tests look much better.

@mantoni mantoni merged commit ec74e94 into sinonjs:master Aug 6, 2017
@mantoni
Copy link
Member

mantoni commented Aug 6, 2017

Great tests! Thank you very much 👍

@fatso83
Copy link
Contributor

fatso83 commented Aug 14, 2017

This introduced regression #1526. Mind taking a look at it, @noisecapella?

@noisecapella
Copy link
Contributor Author

Yes I'll take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement semver:minor changes will cause a new minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants