Add optionally() and make isDone and pendingMocks consistent. #723
Conversation
| return this.reply(statusCode, readStream, headers); | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
As on #721, I'm not sure why this is here, presumably from a previous change that forgot to commit this.
There was a problem hiding this comment.
This file is updated every time the tests are run. I'm trying to figure out a good enough way to avoid it muddling with the diffs (probably just gitignore it for a start).
| @@ -810,6 +811,23 @@ var scope = nock('http://api.myservice.com') | |||
| }) | |||
| ``` | |||
|
|
|||
| ## Optional Requests | |||
|
|
|||
| By default every mocked request is expected to be made exactly once, and until it is it'll appear in `scope.pendingMocks()`, and `scope.isDone()` will return false (see [expectations](#expectations)). In many cases this is fine, but in some (especially cross-test setup code) it's useful to be able to mock a request that may or may not happen. You can do this with `optionally()`. Optional requests do not appear in `pendingMocks()`, and `isDone()` will return true for scopes with only optional requests pending. | |||
There was a problem hiding this comment.
Worth mentioning here that "optional requests are consumed like normal ones once they are matched".
| var interceptorList = self.keyedInterceptors[key]; | ||
| var pendingInterceptors = interceptorList.filter(function (interceptor) { | ||
| var persistedAndUsed = self._persist && interceptor.interceptionCounter > 0; | ||
| return !persistedAndUsed && !interceptor.optional; |
There was a problem hiding this comment.
This piece of code (and the one it replaced) is hiding an assumption: when an interceptor has been fully consumed, it's removed from the keyedInterceptors list.
I would like to get rid of that assumption. This however would require a refactor that touches various parts of the codebase. In the spirit of keeping this PR on target, I would instead suggest adding a comment to document the fact and I'll add an issue to refactor it.
| return pendingInterceptors.length > 0; | ||
| }); | ||
|
|
||
| return pendingInterceptorKeys; |
There was a problem hiding this comment.
I see that pendingMocks was incorrectly returning all mocks. I expect that some users of nock may have depended of that behaviour.
Would you mind adding something like allMocks/remainingMocks/activeMocks (naming's hard) that's doing what pendingMocks was previously doing?
There was a problem hiding this comment.
Done - I've added this as activeMocks, since that's literally what it is I think: every mock that might actively handle a request.
|
|
||
| t.ok(scope.isDone()); | ||
| t.end(); | ||
| }); |
There was a problem hiding this comment.
With the code in this PR, I would expect a mock that's both optional and persist to be absent from pendingMocks but intercept requests multiple times.
Something analogous could be said about optional and times(n).
Would adding tests for these combined behaviours be an overkill?
| }); | ||
|
|
||
| test('activeMocks returns optional mocks only before they\'re completed', function(t) { | ||
| nock.cleanAll(); |
There was a problem hiding this comment.
I'd quite like to add a beforeEach() and do this cleanAll() before every test automatically, but that requires restructuring this whole file (at least) to wrap it all in a test scope, and I thought that was extending this PR a bit too far (especially since it makes the diff unreadable). Might be worth doing everywhere in future though, to make the tests a bit more isolated from one another.
There was a problem hiding this comment.
The big idea is to drop tap in favour of mocha. If you're interested in helping out, drop by our Gitter channel. I plan on starting work on it over the weekend.
|
All your markups sound sensible to me, so I've done the lot. Let me know if there's any other changes you need. |
|
Thanks a lot for contributing to keep nock moving forward. |
|
Published as part of v8.2.0 🎉 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you! |
This PR adds
optionally(), so you can define mocks that might happen, but not worry about it if they don't (I'm using this for some common test setup where for almost every test in need a basic request mocked out, but not for all of them).Along the way I noticed
isDone()andpendingMocks()weren't actually consistent. OnlyisDone()ever considered persisted mocks to be 'done' (they stayed inpendingMocks()forever), and onlyisDone()supportedoptions.requireDone(see below). I've now made them consistent so, tidied the implementations together, and simplified this significantly on the way.There was an existing feature sort-of like
optionally()already present: you could passrequireDone: falseas an option to an interceptor. Passing as an option is less nice UX for my case, but more problematically it was previously inconsistent between pendingMocks (which ignored it) and isDone (which didn't), it wasn't undocumented anywhere, and it doesn't appear to have ever been used (no results in google or in code on github that aren't from direct copies of this codebase). I've removed this, sinceoptionally()does the same thing. Could keep it, your call, but I'd just let it go IMO.