-
-
Notifications
You must be signed in to change notification settings - Fork 736
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
feat: allow passing a function to enableNetConnect()
#1889
feat: allow passing a function to enableNetConnect()
#1889
Conversation
*/ | ||
function enableNetConnect(matcher) { | ||
// TODO-12.x: Replace with `typeof matcher === 'string'`. | ||
if (_.isString(matcher)) { | ||
allowNetConnect = new RegExp(matcher) | ||
} else if (matcher instanceof RegExp) { | ||
allowNetConnect = matcher | ||
} else if (matcher instanceof Function) { | ||
allowNetConnect = { test: matcher } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also make the internal variable a function, but this seemed quite elegant - let me know if you want me to change this.
tests/test_net_connect.js
Outdated
@@ -19,8 +19,7 @@ describe('`disableNetConnect()`', () => { | |||
|
|||
await assertRejects( | |||
got('https://other.example.test/'), | |||
Error, | |||
'Nock: Disallowed net connect for "other.example.test:443/"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bug I mentioned in the PR description - this last parameter is used for displaying a message when the assertion fails, not to check the error itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Nock already has a big API so it’s worth considering carefully before adding even more, though I think this is one is worth it! If we do change the way we handle configuring the unmocked behavior, this might change, though using a function for this seems like the nicest way to express more complicated scenarios than anything else we’ve seen.
|
||
await got('https://example.test/').catch(() => undefined) // ignore rejection, expected | ||
|
||
expect(matcher).to.have.been.calledOnceWithExactly('example.test:443') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
As mentioned in #1889, it appears that `assertRejects` is not currently used correctly - the third parameter is the message that the assertion emits if it fails, not a matcher for the exception text. This PR fixes all usages of `assertRejects` to correct expect on the error message.
enableNetConnect()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even included updates to the types!
Should we send this out as a minor version, or merge it into |
nvm, I just saw this comment #1896 (comment) I'll update the base branch and bring it up to date. |
🎉 This PR is included in version 11.9.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
* feat(1861): allow passing a function to enableNetConnect * Add test to cover passed parameters
Fixes #1861
I've also uncovered a bug in the usage of
assertRejects
- the third parameter is not used by the library to match on the error message, but rather the message to print when the expected rejection does not happen. I will create a separate PR to fix all usages ofassertRejects
in the project.