-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Ability to dynamically match mocks #6701
Ability to dynamically match mocks #6701
Conversation
@prowe: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
Are there any additional steps I should take beyond reporting a feature request and opening a PR to have this looked at/discussed. This is my first Pull Request to this project so I may not be following the right process and just want to make sure i'm doing the best I can. Thanks |
Is there any progress on reviewing/merging this? This would greatly help with my needs for mocking in my application. |
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 have not ran the code, but the changes seem reasonable to me according to my knowledge of the codebase (which is minimal). Additionally, this feature is greatly needed for one of my projects, so I approve.
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.
Agree this PR would be very helpful. Would help on a project I'm on at the moment too.
Co-authored-by: Benjamin Pearson <[email protected]>
Rebased and should be good. Let me know if there is anything else I can do to be of assistance. |
whoa this PR looks great! Can we already start using it ? Do you think it will land in master ? Thank you ! |
I think this is a solid PR and would love to get this in our 3.8 release! I've updated the base branch to our |
Hey @prowe! Would you be willing to rebase this against our |
ccbe0ad
to
4a0bbb8
Compare
8211fa6
to
b863dc1
Compare
🦋 Changeset detectedLatest commit: de539a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I attempted to resolve the merge conflicts here. I'll self-review this later in the week and add a changeset file as well. |
@@ -150,6 +150,42 @@ it("renders without error", async () => { | |||
|
|||
</ExpansionPanel> | |||
|
|||
### Dynamic variables |
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 file has been updated several times since this PR was first opened. Some updates were warranted, I tried not to take too many liberties. Critical feedback is welcome 🙏🏻
src/testing/core/mocking/mockLink.ts
Outdated
@@ -94,6 +97,9 @@ export class MockLink extends ApolloLink { | |||
if (equal(requestVariables, mockedResponseVars)) { | |||
return true; | |||
} | |||
if (res.variableMatcher && res.variableMatcher(operation.variables)) { |
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 was one of the conflict areas that was a bit murkier to resolve. I believe this logic is correct since we still want to return true on an exact match first. The tests pass, but a skeptical eye would be helpful here.
/release:pr |
A new release has been made for this PR. You can install it with |
How are things looking here? I see we have a PR release we can use now 🙌🏼 ... Any progress on getting this reviewed/merged so it can be released with v3.9? |
This is very close to being merged :) Hopefully next week, though I'll be out for a few days at a conference. The final to do item is fixing a TS error these changes have caused that's not being reported on CI since it's occurring in test files (we have a separate to do item to fix that). Thanks for your patience! |
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.
Excited to get this released in our first 3.9 alpha, thanks @prowe! 🙌
This has been released in v3.9.0-alpha.0 🎉 |
This PR provides the functionality outlined in apollographql/apollo-feature-requests#245
It adds support for a new property
MockedResponse.variableMatcher
that is a function that takes variables and returns if they should match for this mock. A user cannot specify both this property as well asresponse.variables
in the same mock.It also passes the variables into the
ResultFunction
so that a function is able to use them to dynamically build a response. Passing these parameters also allows for the use of a mock for the result function and then asserting that specific variables were passed. This testing strategy results in a cleaner error message for mutations if the correct data is not passed and allows for partial matches using jests "objectContaining" and others.My team found testing using the MockProvider to be difficult in several situations and these small changes would have greatly improved our ability to test with the ApolloClient.