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

State-full mocking #2229

Merged
merged 28 commits into from
Feb 3, 2021
Merged

State-full mocking #2229

merged 28 commits into from
Feb 3, 2021

Conversation

alexstrat
Copy link
Contributor

@alexstrat alexstrat commented Nov 17, 2020

This PR re-implements mock package to add state to mocking via a MockStore, an accessible and editable store of all mocked values.

The reasons that motivate this change are exposed in #1682. In brief: a "real" graphql schema is state-full: in most cases, a same query will return the same result. Adding a state adds realism, by default and thanks to user's customizations.

API changes

  • Breaking: mock functions does not receive resolver arguments anymore and can't return promise. Use resolvers option to add resolvers (see example in doc Handling *byId fields)
  • Breaking: when preserved, resolvers will not receive plain mock data anymore as source but rather a Ref that can be used to query the store. If you used __resolveType resolver for mocking interfaces and union, rather use __typename directly in mocks (see doc Abstract types).
  • Deprecated: MockList is deprecated, use plain arrays instead (see doc Using lists in mocks)

⚠️ I kept the previous test suite in mocking-compatibility.spec.ts to verify backward compatibly, potentially reveal other breaking changes or catch bugs. I had a look at each one by one:

  • ✅ throws an error if you forget to pass schema ➡️ added validation in 02fd628
  • ✅ throws an error if the property "schema" on the first argument is not of type GraphQLSchema ➡️ added validation in 02fd628
  • ✅ throws an error if second argument is not a Map ➡️ added validation in 02fd628
  • ✅ 🚫 throws an error if mockFunctionMap contains a non-function thingy ➡️ I removed the test in d8b6e32 because it's now ok to put non-function thingy in mockFunctionMap values
  • ✅ mocks the default types for you
  • ✅ lets you use mockServer for convenience
  • ✅ mockServer is able to preserveResolvers of a prebuilt schema
  • ✅ lets you use mockServer with prebuilt schema
  • ✅ does not mask resolveType functions if you tell it not to ➡️ is passing with 5e36e03
  • ✅ can mock Enum
  • ✅ can mock Enum with a certain value
  • ✅ can mock Unions
  • ✅ ✏️ can mock Interfaces by default ➡️ adapted in 1c9350e
  • ✅ can mock nullable Interfaces
  • ✅ can support explicit Interface mock ➡️ fixed in 944ed66
  • ❌ can support explicit UnionType mock ➡️ fails because relies on mock function resolver arguments
  • ✅ throws an error when __typename is not returned within an explicit interface mock
  • ✅ throws an error in resolve if mock type is not defined
  • ✅ throws an error in resolve if mock type is not defined and resolver failed
  • ✅ can preserve scalar resolvers
  • ✅ can mock an Int
  • ✅ can mock a Float
  • ✅ can mock a String
  • ✅ can mock a Boolean
  • ✅ can mock an ID
  • ✅ nullable type is nullable
  • ✅ can mock a nonNull type
  • ✅ nonNull type is not nullable
  • ✅ can mock object types
  • ✅ can mock a list of ints
  • ✅ can mock a list of lists of objects
  • ✅ does not mask resolvers if you tell it not to
  • ✅ lets you mock non-leaf types conveniently
  • ✅ lets you mock and resolve non-leaf types concurrently
  • ✅ lets you mock and resolve non-leaf types concurrently, support promises
  • ✅ lets you mock and resolve non-leaf types concurrently, support defineProperty
  • ✅ let you mock with preserving resolvers, also when using logger
  • ✅ let you resolve null with mocking and preserving resolvers
  • ✅ ✏️ lets you mock root query fields ➡️ adapted in a5f5dfd (but leads to a blank test case)
  • ✅ ✏️ lets you mock root mutation fields ➡️ adapted in a5f5dfd (but leads to a blank test case)
  • ✅ lets you mock a list of a certain length
  • ✅ lets you mock a list of a random length
  • ✅ 🚫 lets you mock a list of specific variable length ➡️ removed in 3081c0f because was testing a feature (arguments in mock functions) does not exist anymore
  • ✅ lets you provide a function for your MockList
  • ✅ throws an error if the second argument to MockList is not a function
  • ✅ lets you nest MockList in MockList ➡️ Fixed in e605620 and 8e6f456
  • ✅ 🚫 lets you use arguments in nested MockList ➡️ removed in 3081c0f because was testing a feature (arguments in mock functions) does not exist anymore
  • ✅ 🚫 works for a slightly more elaborate example ➡️ removed in 3081c0f because was testing a feature (arguments in mock functions) does not exist anymore
  • ✅ works for resolvers returning javascript Dates
  • ✅ 🚫 allows instanceof checks in __resolveType ➡️ removed in d0aafd4 because __resolveType does not receive anymore as source plain object returned by mock, which makes the test irrelevant
  • ✅ should preserve resolvers for custom scalars if preserveResolvers: true
  • ✅ should work with casual and MockList
  • ✅ 🚫 should merge resolved result of a promise with default mock if available ➡️ removed in 0c5c3ab because mock can't be promise anymore, so irrelevant
  • ✅ resolves subscriptions only once ➡️ Fixed in 7bfe49a

TODOs

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • fix the 2 bugs revealed by existing tests: missing subscription support and issue with list of lists
  • write equivalents of the existing tests that fail because of API change
  • add parameters validations (?) to make these parameters validation tests pass
  • add a migration guide in docs/mocking.md
  • write few paragraphs in docs/mocking.md about MockStore which is the core of the feature
  • backport https://github.com/alexstrat/graphql-stateful-mock/issues?q=label%3Ato-backport-in-graphql-tools+

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2020

🦋 Changeset detected

Latest commit: a1cc0fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@graphql-tools/mock Major
graphql-tools Patch

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

export const randomListLength = () => {
// Mocking has always returned list of length 2 by default
// return 1 + Math.round(Math.random() * 10)
return 2;
Copy link
Contributor Author

@alexstrat alexstrat Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice this: for backward compatibility, i make it return 2 - but it would be richer if we'd truly return a random number.

@alexstrat
Copy link
Contributor Author

alexstrat commented Nov 17, 2020

Waiting for maintainers directions on what I should do with test cases failing because of breaking changes

@ardatan
Copy link
Owner

ardatan commented Nov 26, 2020

@yaacovCR What do you think?

Copy link
Collaborator

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not used the mock package to much, but in getting the general idea, I think this is fantastic. Do we want to revisit when more of the existing tests have parallels in the new setup? Should we have a migration guide?

@alexstrat
Copy link
Contributor Author

I have not used the mock package to much, but in getting the general idea, I think this is fantastic. Do we want to revisit when more of the existing tests have parallels in the new setup? Should we have a migration guide?

All right! I can follow these steps:

  • fix the 2 bugs revealed by existing tests: missing subscription support and issue with list of lists
  • write equivalents of the existing tests that fail because of API change
  • add parameters validations (?) to make these parameters validation tests pass
  • add a migration guide in docs/mocking.md
  • write few paragraphs in docs/mocking.md about MockStore which is the core of the feature

@alexstrat alexstrat marked this pull request as draft December 16, 2020 17:55
}
}
})
});
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome stuff. Huge improvement over current mocking package, which I actually don’t find to be more useful than just simple resolvers plugged into an executable schema.

This looks like a great addition to v8

@alexstrat
Copy link
Contributor Author

alexstrat commented Jan 19, 2021

I'm having an issue with the test "can support explicit UnionType mock": the test is expecting I call (and get values) from an union mock while I'm getting a field value for a type that, in some cases, is a member of a this union.

I can't do it with current impl and don't see any way to do it.

Except this, the rest is ready for review.

@alexstrat alexstrat marked this pull request as ready for review January 19, 2021 19:10
@ardatan
Copy link
Owner

ardatan commented Jan 19, 2021

Awesome! Could you rebase conflicts and add a changeset? I think major release would be needed.

@alexstrat
Copy link
Contributor Author

alexstrat commented Jan 20, 2021

Awesome! Could you rebase conflicts and add a changeset? I think major release would be needed.

@ardatan Done! Note the CI is failing also because of the one test case i mentioned here but I'll need guidance for the rest.

@yaacovCR
Copy link
Collaborator

This is fantastic stuff as far as I can tell from afar. I am not a user, however, of the current mock package so I am not personally in best place to review. :)

Just in general, I personally dont think we need absolutely every existing test to pass -- we just want to give users a better mocking experience, and can make breaking changes as long as we semver right. I think you had the best possible approach in trying to preserve as much of the old functionality as possible -- but may be ok to merge without every old test....

I may have some time over the weekend to take a look at what's failing in more detail and add more thoughts, but like I said I am not too familiar right now with the mock code base... Will defer to others :)

@ardatan
Copy link
Owner

ardatan commented Feb 3, 2021

@alexstrat Skipped the failing test for now! This is awesome feature! Would you like to write a blog post about new mocking features in our blog?
https://github.com/the-guild-org/the-guild-website/tree/master/pages/blog

cc @Urigo

@ardatan ardatan merged commit 8f7918b into ardatan:master Feb 3, 2021
@alexstrat
Copy link
Contributor Author

@ardatan yes, I can submit a draft. Any particular objective for the blog post, or approach?

@ardatan
Copy link
Owner

ardatan commented Feb 3, 2021

@alexstrat You can just mention new changes, covered use cases and maybe a comparison between old approach and the new one :) After you submit a draft, we can help you before publishing it.

ardatan added a commit that referenced this pull request Feb 4, 2021
* Replace old by new implementation

* Move old test suite to retro-compatibility test suite

* Add documenation

* Make randomListLength always return 2 for retro-compatibility

* Fix enum mocking

* Make error messages backward compatible

* Fix mockResolver to give priority to default resolved value

* Complete arrays of non composite types with values

* Add argument validation thrown errors

* Remove test about mockFunctionMap containing a non-function thingy because it's now supported

* Add support for subscription

* Better support for inserting lists of lists

* Deep resolve mock list to better suport MockList of MockList

* Adapt test 'can mock Interfaces by default'

* Fix support for explicit Interface

* Added doc about MockStore

* Added a migration paragraph

* Fix typo

* Fix setting id in union mocks

Backport of alexstrat/graphql-stateful-mock#6

* Expose schema in IMockStore

Backport of alexstrat/graphql-stateful-mock#5

* Remove test because irrelevant

* Removed test because irrelevant

* Removed tests because irrelevant

* Adapt 2 tetsts on root fields

it leads to useles test caeses

* Add changeset

* Fix typo

* Fix GraphQL 14 compability issue

* Skip failing test

Co-authored-by: Arda TANRIKULU <[email protected]>
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.

4 participants