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 mustCall feature #786

Merged
merged 1 commit into from Dec 14, 2017
Merged

Add mustCall feature #786

merged 1 commit into from Dec 14, 2017

Conversation

@geek
Copy link
Member

geek commented Nov 8, 2017

Requested by @cjihrig

@geek geek requested a review from cjihrig Nov 8, 2017
@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Nov 9, 2017

Shouldn't it be part of code ?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Nov 9, 2017

This feature has two parts - first you declare what functions have to be called, then at the end of the test you have check that it was called. The problem with adding it to code is that you'd have to remember to explicitly check at the end of the test. This could go wrong if you forget or have a logic error in your test.

I don't know if it matters, but FWIW, this is how it's handled in Node core as well. Test writers mark functions as mustCall, and then it is automatically checked at the end of the test.

@cjihrig
cjihrig approved these changes Nov 9, 2017
Copy link
Contributor

cjihrig left a comment

LGTM. And thank you for adding this!

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Nov 9, 2017

Sinon does it with 2 (or more) parameters to allow it to keep the function binding, shouldn't we be doing that ?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Nov 9, 2017

I don't think that's necessary. This is the API being implemented. The function is wrapped like this, so context and arguments are all preserved. Here is a simple example of how this is used in Node core.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Nov 9, 2017

This makes sense to me– not dissimilar to plan.

@corbinu

This comment has been minimized.

Copy link
Contributor

corbinu commented Dec 13, 2017

Anything I can do to help with this?

@geek geek self-assigned this Dec 14, 2017
@geek geek added the feature label Dec 14, 2017
@geek geek added this to the 15.2.0 milestone Dec 14, 2017
@geek geek merged commit c6bac7c into hapijs:master Dec 14, 2017
2 checks passed
2 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek geek deleted the geek:mustcall branch Dec 14, 2017
@geek

This comment has been minimized.

Copy link
Member Author

geek commented Dec 14, 2017

@corbinu thanks, I'll include this with the next release

@corbinu

This comment has been minimized.

Copy link
Contributor

corbinu commented Dec 15, 2017

@geek Great! noticed good has a test that could use this during my update

@corbinu

This comment has been minimized.

Copy link
Contributor

corbinu commented Jan 8, 2018

@geek Just checking in on if you know when the new release might be? Was going to do some more updates to good and wanted to update the tests to use this also. Totally understand we all get super busy though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.