apollo-client: 3.8.x has broken tests of components that return different tags based on loading state

Issue Description

First of all: I realize this sounds extremely obscure (and it’s very likely that apollo-client just triggered a latent bug in something else, but maybe we can figure out where / why). It occurred on the 3.7.17 -> 3.8.0 upgrade (it just took me so long to minimize it).

If the basic Dog component on https://www.apollographql.com/docs/react/development-testing/testing/ is altered to return a <div> on success like so:

 export function Dog({ name }) {
   const { loading, error, data } = useQuery(GET_DOG_QUERY, {
     variables: { name }
   });
   if (loading) return <p>Loading...</p>;
   if (error) return <p>{error.message}</p>;
   return (
-    <p>
+    <div>
       {data.dog.name} is a {data.dog.breed}
-    </p>
+    </div>
   );
 }

The test checking both the loading and success state fails like so (jest, jsdom-environment):

$ npm test

> apollo-client-error-template@1.0.0 test
> jest

 FAIL  src/dog.test.jsx
  ✕ should render dog (50 ms)

  ● should render dog

    expect(element).toBeInTheDocument()

    element could not be found in the document

      19 |     </MockedProvider>
      20 |   );
    > 21 |   expect(await screen.findByText("Loading...")).toBeInTheDocument();
         |                                                 ^
      22 |   expect(await screen.findByText("Buck is a poodle")).toBeInTheDocument();
      23 | });
      24 |

      at toBeInTheDocument (src/dog.test.jsx:21:49)
      at call (src/dog.test.jsx:2:1)
      at Generator.tryCatch (src/dog.test.jsx:2:1)
      at Generator._invoke [as next] (src/dog.test.jsx:2:1)
      at asyncGeneratorStep (src/dog.test.jsx:2:1)
      at asyncGeneratorStep (src/dog.test.jsx:2:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.291 s, estimated 2 s
Ran all test suites.

Attention if the component is modified to return a wrong string (e.g. dropping the article), the test fails as one would expect:

Error when the component returns the wrong string

npm test

> apollo-client-error-template@1.0.0 test
> jest

 FAIL  src/dog.test.jsx
  ✕ should render dog (1088 ms)

  ● should render dog

    Unable to find an element with the text: Buck is a poodle. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

    Ignored nodes: comments, script, style
    <body>
      <div>
        <div>
          Buck
           is 
          poodle
        </div>
      </div>
    </body>

      20 |   );
      21 |   expect(await screen.findByText("Loading...")).toBeInTheDocument();
    > 22 |   expect(await screen.findByText("Buck is a poodle")).toBeInTheDocument();
         |                       ^
      23 | });
      24 |

      at waitForWrapper (node_modules/@testing-library/dom/dist/wait-for.js:162:27)
      at node_modules/@testing-library/dom/dist/query-helpers.js:86:33
      at findByText (src/dog.test.jsx:22:23)
      at call (src/dog.test.jsx:2:1)
      at Generator.tryCatch (src/dog.test.jsx:2:1)
      at Generator._invoke [as next] (src/dog.test.jsx:2:1)
      at asyncGeneratorStep (src/dog.test.jsx:2:1)
      at asyncGeneratorStep (src/dog.test.jsx:2:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        2.392 s
Ran all test suites.

When downgrading to apollo client 3.7.17, the issue vanishes.

Link to Reproduction

https://github.com/gzm0/apollo-client-different-tag-repro

Reproduction Steps

The reproduction repositories commit history shows the steps:

Clone template.
Setup dog test with jest
Change p to div, observe failure.
Downgrade to first failing apollo-client version.

About this issue

Original URL
State: closed
Created 9 months ago
Comments: 18 (16 by maintainers)

Commits related to this issue

Add delay in docs for loading state testing Following the discussion in #11285. — committed to gzm0/apollo-client by gzm0 9 months ago

Most upvoted comments

Looks like this is needed after the changes in 3.8.x to avoid act() warnings on tests that used to run fine on prior versions, even if you are not trying to test the loading state. Is this expected? @phryneas

For example, any test that tries to verify that refetch is called (refetch is a mock jest function passed as newData property in the mock) now prints act warnings to the console. The same exact tests used to work fine before without the need for any delay

Honestly: hard to say. We are at the whim of what React does there - React 17 did it differently from React 18, and React 19 will likely have different timing again (I think canaries, e.g. Next.js, might already have this, but I could be wrong).
Right now we have moved the library more into the direction of how the React team intended the API to be used, and I would personally argue that a call to a useSyncExternalStore update function should never trigger an act warning, since it usually won’t be triggered by user interaction that can be wrapped in act - but it’s pretty much out of our control.

In the end the only advise I can probably give you here is: do what’s working for you - even if that’s a bit bitter 😞

On the upside: you’ll never have a network with zero latency, so maybe adding that delay actually makes your tests more realistic.
(Also, once you switch over to useSuspenseQuery, all of these sorrows should be out of the window)

phryneas on Nov 7, 2023

Done 😃

gzm0 on Oct 16, 2023