Add filterCalls(), testCalls()

[motiz88]

This combines a proposed implementation of #88 with a doc fix for #87 which originally prompted me to come up with this API. URL matcher compilation was refactored so it could be shared between compileRoute and the new methods.

Notes:

• I originally called these functions callsMatched() and calledMatched(), but in light of @wheresrhys's concerns, I now think it would be better to distance them from calls() and called() to reduce confusion, hence their new names.
• This bakes normalizeRequest into the handling of matchers, in the new methods only (see compileUserUrlMatcher). This may or may not be desired but was the quickest way to get roughly what I wanted here.

EDIT: I've rewritten most of this description for clarity. Further discussion of the separate motivations for this PR probably belongs separately in issues #87 and #88.

[wheresrhys]

Nice idea, thanks for submitting

One edge cases possibly not covered (don't have time to review the code properly this second) is what about if two matchers match a given url. fetch mock will respond to the first one matched, but what if in your test you check callsMatching for the second one. Could lead to hard to trace mistakes in the tests.

I read called(matcher) to mean a matcher like '^http://foo' would work, which it doesn't

That's weird - it's supposed to (unless I changed the behaviour sometime and can't remember the details any more). Aside from getting around this bug/unhelpful behaviour, do you see any other advantages with your approach? Any use cases in mind?

I don't particularly like the way it is now either - was a bit of a compromise for v2/3 to avoid requiring users to name every route. I think it needs some careful thought though before changing the way it works again.

[wheresrhys]

I've written a test to cover the ^http://foo case which wasn't working for you, and the test passes, so I'm not sure what could have been happening in your case https://github.com/wheresrhys/fetch-mock/compare/rhys/unnamed-route-tests?expand=1

[wheresrhys]

I've written a test to cover the ^http://foo case which wasn't working for you, and the test passes, so I'm not sure what could have been happening in your case https://github.com/wheresrhys/fetch-mock/compare/rhys/unnamed-route-tests?expand=1

[motiz88]

Let me be clearer about where my expectations conflicted with the actual API.

First, in code:

fetchMock.mock('^http://example.com/');
fetch('http://example.com/one/two').then(() => {
  fetchMock.called('^http://example.com/'); // true
  fetchMock.called('^http://example.com/one'); // false (surprising to me)
  fetchMock.called(/one\/two$/); // false (surprising to me)
  fetchMock.calledMatching('^http://example.com/'); // true
  fetchMock.calledMatching('^http://example.com/one'); // true
  fetchMock.calledMatching(/\/one\/two$/); // true
});

As you see, I thought that I could meaningfully pass arbitrary RegExps or ^-patterns, not just ones that I'd previously configured as routes, to called() (and calls()). (I also think that this is a useful option to have, as a complement if not substitute to the existing methods - which is why I wrote the new ones.)

I think the docs contributed to that misunderstanding, as the argument name matcher was reused to describe two quite different entities: an actual matcher (string|object|RegExp|function) vs. a route name (inferred or explicit). That's why, in addition to extending the API, I propose a doc change from matcher to name where the only valid input is a route name.

And, come to think about it - Is it ever useful to pass an unknown route into calls/called/etc? If not, you might also want to explicitly mark that usage as incorrect by emitting an error/warning accordingly.

[motiz88]

what about if two matchers match a given url. fetch mock will respond to the first one matched, but what if in your test you check callsMatching for the second one. Could lead to hard to trace mistakes in the tests.

I'm afraid I don't fully understand what you mean here. Do you mean something like this?

fetchMock.mock('^http://hey')
  .mock('http://hey.there');

fetch('http://hey.there') // or anything that would match a route (or routes)
.then(() => {
  fetchMock.calledMatching(/hey/); // true
  fetchMock.callsMatching(/hey/); // {routed: [['http://hey.there', opts]], unrouted: []}
});

[motiz88]

The thing is, callsMatching absolutely doesn't care how the URL was originally matched or what routes are configured, other than that they caused a call to be pushed to either _matchedCalls or _unmatchedCalls (so it can classify them into routed/unrouted).

[wheresrhys]

Here is the case I'm concerned about

fetchMock
  .mock('http://hey.there')
  .mock('^http://hey')

fetch('http://hey.there') // or anything that would match a route (or routes)
.then(() => {
  fetchMock.calledMatching('^http://hey'); // true
  fetchMock.called('^http://hey'); // false
});

Here's where I stand on this at the moment

1. You're right, the docs could be clearer
2. I can see the benefits of your approach, but also worry about the edge case above, and potentially others we may not have thought of
3. I don't particularly want to have two supported ways of checking for matched calls - that will add to the complexity of the api, and thus increase confusion when it's being used. So if your suggestion was adopted I see it being a major release
4. No other user has reported confusion/problems with how called() behaves

Given those points, I think updating the docs and opening an issue with a title like 'Behaviour of called(matcher) is confusing', referencing this PR, would be a good way to see if there is demand for a change in behaviour.

[wheresrhys]

Thanks for raising those issues