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

fix: allowMocked when using a callback for the path #1877

Merged

Conversation

mastermatt
Copy link
Member

When an Interceptor was created with a comparator for the path, the
matchOrigin function was comparing the string equivalent of the function
instead of evaluating it.

I'm not a fan of the fact that the function matchOrigin is comparing the
pathname, but that's a refactor for another day.

Fixes: #1867

When an Interceptor was created with a comparator for the path, the
`matchOrigin` function was comparing the string equivalent of the function
instead of evaluating it.

I'm not a fan of the fact that the function `matchOrigin` is comparing the
pathname, but that's a refactor for another day.

Fixes: nock#1867
@mastermatt mastermatt merged commit 9fdeeca into nock:master Feb 10, 2020
@mastermatt mastermatt deleted the 1867/fix-allowunmocked-verb-callback branch February 10, 2020 04:41
@nockbot
Copy link
Collaborator

nockbot commented Feb 10, 2020

🎉 This PR is included in version 11.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@YaroslavRepeta
Copy link

@mastermatt This fix added another bug (or at least unexpected behavior).
Function for checking path is called twice now (in matchOrigin and match functions).
The easiest fix could be to move https://github.com/nock/nock/blob/master/lib/intercept.js#L398-L400
below under if (allowUnmocked).
Would you be able to fix that?

@mastermatt
Copy link
Member Author

@YaroslavRepeta validating existence of matches must happen before checking for the allowUnmocked flag for that logic to work.

The function being called multiple times was expected, note my comment about matchOrigin comparing the pathname. Is there a reason why calling a provided comparator multiple times per request is a problem? Any provided callback should not have side effects.

@YaroslavRepeta
Copy link

@mastermatt But matches is not used at all, if allowUnmocked=false. What I proposed is:

if (allowUnmocked) {
  const matches = ...;
  if (!matches) {

instead of

const matches = ...;
if (!matches && allowUnmocked) {

I use a function with counter to simulate server what returns success for first N requests only. And this change breaks my code. However I understand that it's not something what this library guarantee.

@mastermatt
Copy link
Member Author

But your suggested change would fundamentally change/break how allowed un-mocked requests would work. We must decide if we have any matches first before falling back to un-mocked.

For your use case you should be using conditionally instead.
As I mentioned in my earlier comment, this callback should be pure and not have side effects.

@YaroslavRepeta
Copy link

YaroslavRepeta commented Apr 2, 2020

It's not about the whole library, it's about that piece of code:
For that function (https://github.com/nock/nock/blob/master/lib/intercept.js#L398-L400), there are two ways of execution:

if (!matches && allowUnmocked) {
   return A;
}
return B;

If we change to:

if (allowUnmocked) {
  const matches = ...;
  if (!matches) {
    return A;
  }
}

return B;

nothing will happen, except the fact of saving on unneeded extra computation of matches.

Actual checking matches is done in another place:
https://github.com/nock/nock/blob/master/lib/interceptor.js#L326-L331

But ok, I'll try to use conditionally, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allowUnmocked option doesn't work properly for specified paths
4 participants