Issue 5917 mocked provider onerror

[yuhaoju]

Which problem does this PR solve?

MockedProvider doesn't log error when mock is missing, and provides no way to inspect the queries being made during a test. Issue: #5917

Main changes

1. Add onError() prop to MockedProvider
2. Pass onError() from MockedProvider to MockLink through param.
3. In MockLink, check if custom error handler is defined and if it should run it.

Checklist:

• If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• If this PR is a new feature, please 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

I will add unit tests if these changes look good.

[apollo-cla]

@yuhaoju: 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/

[changeset-bot]

🦋 Changeset detected

Latest commit: 31fd91e

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

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Minor

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

[jerelmiller]

Hey @yuhaoju! Appreciate the contribution!

I'm a bit concerned around this particular solution to the linked issue, particularly because it puts to onus on determining when there might be an issue back on the developer writing the test. This feels a little bit like we are asking the developer to pass this function so that you might "hope" to catch something. 95% of the time, that is just going to be added noise in your test with no benefit.

Instead, I think it would make sense to log a warning to the console so that its seen in the test itself. This seems to match the ask from issue:

This cause my tests, which used to pass, to suddenly fail silently, and gives me a hard time to find the cause

I also think this statement nails it:

To me, it seems that most of the time, this error would not be intentional, and that the expected behavior isn't to send it to the component as a real world error, but instead would be to display a warning in the unit test.

Not matching a mock is a user-error mistake, and we should do our best to let you know something is misconfigured. The console warning would really help in that case.

Would you instead be willing to pivot and display a console warning instead? I'd love for you to get contribution credit if you're still interested in contributing a solution. If not, no worries! We can close this PR and implement it ourselves.

Let me know what you think!

[jerelmiller]

(my previous comment has my feedback. Forgot to submit this as a formal review 😬 )

[jerelmiller]

Hey @yuhaoju!

Just wanted to give a gentle nudge and see if you'd be interested in moving this forward. No problem if not. Thanks!

[yuhaoju]

@jerelmiller Sorry for the late reply. I think in my case I do want to format the error message, like using jest-diff to compare and display mocked request params. There are some complex requests, and sometimes it's hard to find the difference from strings. But I also agree that it can be extra noise for the rest of scenarios, I guess displaying a warning message should be a better idea.

So please feel free to close this PR and open another one. Thanks for your reply, I'd like to contribute if there is chance in the future.

[jerelmiller]

@yuhaoju totally understand your thinking here, I'm just not sure its the direction we want to move for the library itself. If you'd still like that ability, you might consider copying over the MockedProvider and/or MockLink and make those modifications yourself in your own codebase and use throughout your tests.

Thanks for the reply though and the contribution!