Configure behaviour for missing request handlers

mykolavlasov commented 9 months ago

Motivation

The current implementation of a library throws an error if at least one executed query wasn't mocked.

For example, when using Apollo together with React you could have a <Component1 /> that is using queryOne, and <Component2 /> which is a deep child of a <Component1 /> and is using queryTwo. I'm writing a unit test for <Component1 /> and mocking a response for a queryOne. However, my test will fail because I haven't provided a mock for a queryTwo with smth like Error: Request handler not defined for query: query queryTwo. I've attached a sandbox with a reproduction of this issue. It's very inconvenient to provide mocks for all requests because the rendered tree could be huge, so you should know the implementation of your children to do it, and should frequently update in case of changes.

Additional motivation for this change:

Align with apollo-client/testing MockedProvider -> MockLink implementation where an error is thrown inside of the returned observable.
This change returns behaviour regarding throwing an error to how it was before the 1.1.0 version. In 1.1.0 you introduced subscription but also changed how errors are thrown.

Description of change

Throw an error inside of the observable that is returned from the request method instead of throwing it directly in the function body.

How to reproduce

Sandbox where you can reproduce the issue. Login, fork and run npm run test in the sandbox terminal. You can uncomment mock for a second query or downgrade to mock-apollo-client@1.0.0 to fix the issue in a sandbox

mykolavlasov commented 8 months ago

@Mike-Gibson Could you please take a look? Sorry for tagging, but for some reason I cannot assign you as a reviewer of this PR

mykolavlasov commented 7 months ago

@Mike-Gibson Could you please check this PR?

Mike-Gibson commented 7 months ago

Hi @mykolavlasov

Apologies for not getting back to you sooner - I've had limited time recently to spend on personal projects.

Thanks for putting together a really detailed PR with the codesandbox example. I can see the difference in behaviour you mention, and linking to the official apollo mock link is really helpful.

I can see the benefit of the change you're proposing in this example. I do wonder if it will make it more difficult to diagnose when there is a legitimate missing request handler though. Say a developer had a typo in the mocked request, or forgot about a query that needed to be mocked, the only way they would find out would be to debug the test/add logging in the component error handler which I think would add friction. At least the current behaviour logs the 'missing handler' error in the console, albeit via an unhandled error.

Another thought I have around your example, it that I believe there are two schools of thought when it comes to testing hierarchies of components - one where the individual component is tested in isolation, and another where the entire tree is tested together. The situation you're describing above is where the parent is being tested in isolation. In this case, I would have probably expected the child component itself to be mocked out/stubbed in the test. As you could also imagine situations where the child component needs some other React context to be available, or some other network requests are made which also need mocking out.

It's tricky for me, as I'm not sure what the most useful behaviour for the majority of people would be.

If you think it would actually be useful to allow missing handlers, is there perhaps an alternative to support both sets of behaviour - the first which will make it very clear (to the developer) when there's an unintentional missing query, and the second case you mention - where actually you don't mind (or expect) there is a missing query, and wouldn't want to be told about it. (In this case - maybe it's better not to return an error, but leave it in a loading state?)

What about having an additional option in MockApolloClientOptions which allows the developer to opt out of the missing handler behaviour? We could also perhaps use the change you've proposed and in addition to returning the error from the link, log a warning to the console - this would then hopefully keep it clear that a handler is missing.

Would really appreciate your thoughts!

mykolavlasov commented 7 months ago

Hi @mykolavlasov

Apologies for not getting back to you sooner - I've had limited time recently to spend on personal projects.

Thanks for putting together a really detailed PR with the codesandbox example. I can see the difference in behaviour you mention, and linking to the official apollo mock link is really helpful.

I can see the benefit of the change you're proposing in this example. I do wonder if it will make it more difficult to diagnose when there is a legitimate missing request handler though. Say a developer had a typo in the mocked request, or forgot about a query that needed to be mocked, the only way they would find out would be to debug the test/add logging in the component error handler which I think would add friction. At least the current behaviour logs the 'missing handler' error in the console, albeit via an unhandled error.

Another thought I have around your example, it that I believe there are two schools of thought when it comes to testing hierarchies of components - one where the individual component is tested in isolation, and another where the entire tree is tested together. The situation you're describing above is where the parent is being tested in isolation. In this case, I would have probably expected the child component itself to be mocked out/stubbed in the test. As you could also imagine situations where the child component needs some other React context to be available, or some other network requests are made which also need mocking out.

It's tricky for me, as I'm not sure what the most useful behaviour for the majority of people would be.

If you think it would actually be useful to allow missing handlers, is there perhaps an alternative to support both sets of behaviour - the first which will make it very clear (to the developer) when there's an unintentional missing query, and the second case you mention - where actually you don't mind (or expect) there is a missing query, and wouldn't want to be told about it. (In this case - maybe it's better not to return an error, but leave it in a loading state?)

What about having an additional option in MockApolloClientOptions which allows the developer to opt out of the missing handler behaviour? We could also perhaps use the change you've proposed and in addition to returning the error from the link, log a warning to the console - this would then hopefully keep it clear that a handler is missing.

Would really appreciate your thoughts!

Thanks for answering!

You're completely right about adding reasonable logs for a missing query - highlighting specific errors is always better than silently failing.

two schools of thought when it comes to testing hierarchies of components

I'll be speaking about React since I lack experience with other UI libs/frameworks:

the individual component is tested in isolation - It was quite popular to use shallow rendering(automatically mock child components during rendering) from tools like Enzyme. But Enzyme is no longer maintained and doesn't support React versions > 16 (there's an unofficial adapter for 17 version that is also unmaintained), and it is a rather deprecated project. Enzyme + React 17 issue on GH
the entire tree is tested together - this is the most used approach based on my experience, and also a recommended way by a de-facto industry-standard testing-library that doesn't support shallow rendering and renders the whole tree. While it might be more complex to setup(wrapping components with providers, etc), it better resembles how a real user might interact with an app(compared to shallow rendering).

mykolavlasov commented 7 months ago

I made changes to this PR and introduced a configuration field, missingHandlerPolicy that allows flexibility to configure how to behave on missing handler:

throw-error - throws an error during test. Default value to not break backward compatibility
warn-and-return-error - logs a warning to a console and returns an error.
return-error - returns an error silently, not logs are printed.

Please let me know what you think about it. I'll update the README if you're good with this implementation @Mike-Gibson

Mike-Gibson commented 7 months ago

@mykolavlasov these changes look great - I really like the new configuration field, and really appreciate the update to the tests. If you could update the README that would be awesome, thanks!

mykolavlasov commented 7 months ago

@mykolavlasov these changes look great - I really like the new configuration field, and really appreciate the update to the tests. If you could update the README that would be awesome, thanks!

I've updated the docs and would be glad if you could merge it and release a new version. Thanks for your feedback! Please reach me if you think it needs more work.

Mike-Gibson commented 7 months ago

@mykolavlasov I've created a new release - 1.3.0 which includes this change.

Thanks for your contribution. Let me know if you run into any issues.

Mike-Gibson / mock-apollo-client