apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.34k stars 2.66k forks source link

Testing Error State of Mutation is throwing Global Error #7167

Open mkaraula opened 3 years ago

mkaraula commented 3 years ago

I want to reopen https://github.com/apollographql/react-apollo/issues/2614 from the old repo as I ran into the exact same Issue today with @apollo/client": "3.2.2.

I have a component that uses the useMutation hook and renders differently when useMutation returns an error

export const DELETE_DOG_MUTATION = gql`
  mutation deleteDog($name: String!) {
    deleteDog(name: $name) {
      id
      name
      breed
    }
  }
`;

export function DeleteButton() {
  const [mutate, { loading, error, data }] = useMutation(DELETE_DOG_MUTATION);

  if (loading) return <p>Loading...</p>;
  if (error) return <p>Error!</p>;
  if (data) return <p>Deleted!</p>;

  return (
    <button onClick={() => mutate({ variables: { name: 'Buck' } })}>
      Click me to Delete Buck!
    </button>
  );
}

I want to Test this behaviour as described in the Docs

it('should show error UI', async () => {
  const deleteDog = { name: 'Buck', breed: 'Poodle', id: 1 };
  const mocks = [
    {
      request: {
        query: DELETE_DOG_MUTATION,
        variables: { name: 'Buck' },
      },
      error: new Error('aw shucks'),
    },
  ];

  const component = renderer.create(
    <MockedProvider mocks={mocks} addTypename={false}>
      <DeleteButton />
    </MockedProvider>,
  );

  // find the button and simulate a click
  const button = component.root.findByType('button');
  button.props.onClick(); // fires the mutation

  await new Promise(resolve => setTimeout(resolve, 0)); // wait for response

  const tree = component.toJSON();
  expect(tree.children).toContain('Error!');
});

Intended outcome:

The Test should pass without throwing an error.

Actual outcome:

Passing mocks with an Error to <MockedProvider /> does actually throw a global Error

    aw shucks

      at new ApolloError (node_modules/@apollo/client/errors/index.js:26:28)
      at Object.error (node_modules/@apollo/client/core/QueryManager.js:146:48)
      at notifySubscription (node_modules/zen-observable/lib/Observable.js:140:18)
      at onNotify (node_modules/zen-observable/lib/Observable.js:179:3)
      at SubscriptionObserver.error (node_modules/zen-observable/lib/Observable.js:240:7)
      at node_modules/@apollo/client/utilities/observables/iteration.js:4:68
          at Array.forEach (<anonymous>)
      at iterateObserversSafely (node_modules/@apollo/client/utilities/observables/iteration.js:4:25)
      at Object.error (node_modules/@apollo/client/utilities/observables/Concast.js:33:21)
      at notifySubscription (node_modules/zen-observable/lib/Observable.js:140:18)

I figured out that when I add an onError option to useMutation my test runs as expected but I guess this is just a workaround.

  const [mutate, { loading, error, data }] = useMutation(DELETE_DOG_MUTATION, {onError: () => {}});

How to reproduce the issue:

I cant share the code of the application I am working on but I can try to create a Codesandbox if neccessary but I hope my explanation is detailed enough.

Versions System: OS: macOS Mojave 10.14.6 Binaries: Node: 10.16.0 - /usr/local/bin/node npm: 6.11.2 - /usr/local/bin/npm Browsers: Chrome: 86.0.4240.80 Firefox: 81.0.1 Safari: 12.1.2 npmPackages: @apollo/client: ^3.2.2 => 3.2.2 "jest": "^24.9.0",

adamnbowen commented 3 years ago

I'm experiencing the same problem with a useQuery. I'll just avoid testing errors for now and wait for a response on this thread.

sean6bucks commented 3 years ago

Experiencing the same thing. As mentioned in the previous thread, you can work around it by swallowing errors with a try/catch, but i hate adding extra code in my components JUST for the tests to work. I would assume the MockedProvider would know to swallow these errors itself, and expect the returned errors item in the mutation to do its work.

neutraali commented 3 years ago

Same issue, different flavor. Trying to migrate into @apollo/client 3.0.0+. Supplying errors -array to mocks triggers a global error. Supplying error -object to mocks triggers normal error, but is different than the previous <3.0.0 behaviour.

markcnunes commented 3 years ago

This comment managed to get around it using errorPolicy:

<MockedProvider
  mocks={mocks}
  addTypename={false}
  defaultOptions={{
    mutate: {
      errorPolicy: 'all'
    }
  }}
>

You can then target to find the element rendered by if (error) return <p>Error!</p>;

Cretezy commented 3 years ago

@markcnunes Unfortunately this solution breaks the onComplete callback, as it's called even when there's an error (with undefined data).

This seems like a high priority issue.

leepowelldev commented 3 years ago

Just got bitten by this ... wasted a couple of hours. If there's no appetite to change behaviour by the Apollo team, then this behaviour really needs to be highlighted in the documentation. I guess my expectation was any errors would be passed back in the error property, without ALSO throwing.

Seems the best way to handle this is by adding a empty onError handler in the options in any useQuery or useMutation - but it would be great if there was an option to suppress the error also being thrown and just handle it in the error property if needed.

thisfrontenddev commented 2 years ago

I've been struck with the same issue just now and while this seems obvious now I have to admit that documentation isn't clear about this, so here's what's happening: even though the error attribute is populated, the mutate function is still throwing an error (either Error or GraphQLError depending on the nature).

That being said, in this example:

<button onClick={() => mutate({ variables: { name: 'Buck' } })}>

mutate is called in a lambda but the exception is never caught, so it is bubbling up, ending in the test failing as it is catching the error that's thrown. The solution is to swallow the error thrown by the mutation function one way or another. Even though you can get the error in the response object, the error is still being thrown and thus, it has to be taken care of.

Documentation while correct does not give an example that can pass tests.

TLDR; in order to avoid your tests failing, you have to use the mutation in an async/await function, wrapped try/catch, or simply use the Promise API by adding a .catch() method to it.

christo8989 commented 2 years ago

I'm also experiencing this issue.

ColeStansbury commented 2 years ago

+1 Bumping this! Issue has been open for almost 18 months.

bignimbus commented 1 year ago

Thanks all, it seems that some commenters see this as a documentation issue and some see this as a source code issue. @thisfrontenddev what docs page would you recommend changing?

leepowelldev commented 1 year ago

For me, its a lack of documentation around the handling of errors in general for mutations (via useMutation) which is the issue. I had to go through a laborious trial/error trying to understand the scenarios in which a mutation would throw or call the onError option.

This is different to how queries work, where they don't throw at all and instead pass errors back either through the return value of a lazy query, or the error property of a greedy query. I would expect a lazy query and a mutation to behave in a similar way as they have a similar interface.

It is different again if you use the client directly in which queries and mutations throw. Personally I would like to see mutations (via useMutation) not throw if a onError option is not present as that would align them with the query behaviour.

bignimbus commented 1 year ago

Thanks for that response @leepowelldev! In 3.7.4 we did ship a patch to the useMutation callbacks: https://github.com/apollographql/apollo-client/pull/10425

Does that help clear things up?

leepowelldev commented 1 year ago

That just fixed the bug I reported that both sets of onError callbacks are called if both present. I still feel there are lots of inconsistencies on how mutations behave when dealing with errors - and although I realise it would be difficult to change that behaviour, I think the docs should be updated to reflect that behaviour much more clearly. On 14 Feb 2023, at 04:21, Jeff Auriemma @.***> wrote: Thanks for that response @leepowelldev! In 3.7.4 we did ship a patch to the useMutation callbacks: #10425 Does that help clear things up?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

AChangXD commented 1 year ago

@bignimbus So I'm running into something similar, in which a mutation performed using useMutation with Nextjs13/AppDir is throwing a runtime, but does NOT throw a runtime error when error occurs while querying via useLazyQuery'. Is there a particular reason why those two error cases are handled differently? I would think using the error returned byuseLazyQueryanduseMutation` to be the logical step in handling errors, not straight throwing runtime error.

jtonneyck commented 1 year ago

Try throwing a GraphQL error in the results:

    {
      request: {
        query: DELETE_DOG_MUTATION,
        variables: { name: 'Buck' },
      },
     result: { 
         errors: [new GraphQLError('aw shucks')]
    },
   }
  ];

Straight from the docs: https://www.apollographql.com/docs/react/development-testing/testing/#graphql-errors. Probably wasn't there yet when the original question was posted.

jerelmiller commented 1 year ago

@jtonneyck there is actually a difference between the two properties. The top-level error is meant to simulate a network error, where result.errors is meant to simulate errors returned in your GraphQL response itself. Both are treated slightly differently by the client (for example errorPolicy doesn't affect the outcome of a network error).

I agree though, we could have some better documentation around this.

jtonneyck commented 1 year ago

He @jerelmiller For me, in combination with

      defaultOptions={{
        mutate: {
          errorPolicy: 'all',
        },
      }}

I got an error back on:

    const [mutate, { loading, error, data }] = useMutation(DELETE_DOG_MUTATION);

Which is I believe the original motivation behind the question. It served my purpose at least. ;)

jerelmiller commented 1 year ago

Oh ya that will definitely work as well. I just wanted to point out that there is a difference between the 2 error properties and they aren't interchangeable since they impact the behavior in different ways.

Glad this solution works well for you though!

Yimiprod commented 5 months ago

Got bite by the same problem, putting the params didn't solved the problem for testing error from network:

      defaultOptions={{
        mutate: {
          errorPolicy: 'all',
        },
      }}

It works for GraphQLError sent in response tho.