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

WIP: test to show that mocking & resolving cannot be used concurrentl… #96

Merged
merged 3 commits into from
Aug 11, 2016
Merged

WIP: test to show that mocking & resolving cannot be used concurrentl… #96

merged 3 commits into from
Aug 11, 2016

Conversation

sebastienbarre
Copy link
Contributor

@sebastienbarre sebastienbarre commented Aug 6, 2016

Per @helfer suggestion in apollographql/apollo-server#9, submit a PR with a failing test, showing a case where mocks and resolver can't be use concurrently.
Thank you.

@apollo-cla
Copy link

@sebastienbarre: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@helfer
Copy link
Contributor

helfer commented Aug 8, 2016

@sebastienbarre Thanks, I think this is an excellent start. Now we just have to figure out how to make it do what we would expect 😬

According to the comments in the code it should already be doing that (mock.js), so the next step you could take would be to figure out why that test is failing by seeing which branch it takes for mocking the returnString field. The code isn't that straight forward (it's functions wrapping functions), so let me know if you have any questions!

@sebastienbarre
Copy link
Contributor Author

sebastienbarre commented Aug 9, 2016

@helfer thanks. So here is a stab at it. The if (preserveResolvers && field.resolve) was essentially bypassing the mockers, I moved it below so that I can merge the result of a resolver and the result of the mock, if they are both objects. This might look pretty clumsy, I trust you can make it look better through the magic of functional programming. Now I'm not sure what that big block below the we have to handle the root mutation and root query types differently comment does, but since it sets field.resolve, it might require checking preserveResolvers there too. The tests pass, so whatever that block is doing is not being exercised by the tests exercising preserveResolvers, right?
Thanks!

@helfer
Copy link
Contributor

helfer commented Aug 11, 2016

I think this should work! If I remember correctly how mocking is done, it shouldn't be an issue on the root value.

Can your rebase this on master so I can merge it?

@sebastienbarre
Copy link
Contributor Author

@helfer done 👍

@helfer helfer merged commit 47e2368 into ardatan:master Aug 11, 2016
@helfer
Copy link
Contributor

helfer commented Aug 11, 2016

Perfect, thanks!

@sebastienbarre
Copy link
Contributor Author

Unfortunately, this is now broken :( Remember when I mentioned "Now I'm not sure what that big block below the we have to handle the root mutation and root query types differently comment does"? Something had to be done there too. I'll have to come with a new test and fix...

@helfer
Copy link
Contributor

helfer commented Aug 30, 2016

Yes, I remember that. Unfortunately I think I forgot it and merged the PR before I took a look at that.

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

Successfully merging this pull request may close these issues.

None yet

3 participants