3.8.x has broken tests of components that return different tags based on loading state #11285
Comments
|
From that error, it seems to be rendering "Buck is poodle", but not "Buck is a poodle". That's extremely weird, but I can't see how Apollo Client could ever trigger such a React render :/ There were some small internal timing changes in 3.8.0, but only that - rendering is always done by React. This is very weird. We'll try to investigate (thank you for the reproduction!) but I cant guarantee that we'll find anything :/ |
|
Oh, sorry. Maybe this was more confusing than helpful. The second error happens if the component is intentionally broken. It shows that the test seems to reach the second expect. The previous error ("element could not be found in the document") is the one caused by the bug (not any less weird though I guess :-/) Edit: I've moved the secondary error in a |
|
Did you upgrade from react v17 to v18 recently? Our team is in the middle of upgrading to react v18 and we're experiencing a LOT of test failures, especially when testing loading states. It seems the changes that come with Concurrent React break some assumptions we made in the tests. Not sure if that's exactly what's going on here, just thought I would give my experience in the hope that it will be helpful |
|
@dylanwulf TY for that hint. I have attempted a React 17 downgrade (which made the bug go away). But it turns out, downgrading @testing-library/react@13.4.0: OK @apollo/client@3.8.0-beta.7: OK So to trigger the bug, it seems we need at least:
If either (!) of these get downgraded, the bug seems to vanish. Diffs: |
|
I've managed to reproduce this in the apollo-client repository itself (test below) and managed to bisect. The triggering change seems to be: f766e83. It seems that combined with: What I used to bisectCmd: import React from "react";
import gql from "graphql-tag";
import { useQuery } from "../useQuery";
import "@testing-library/jest-dom";
import { render, screen } from "@testing-library/react";
import { MockedProvider } from "../../../testing";
// Make sure that both the query and the component are exported
export const GET_DOG_QUERY = gql`
query GetDog($name: String) {
dog(name: $name) {
id
name
breed
}
}
`;
export function Dog({ name }: { name: string }) {
const { loading, error, data } = useQuery(GET_DOG_QUERY, {
variables: { name }
});
if (loading) return <p>Loading...</p>;
if (error) return <p>{error.message}</p>;
// Change the <div> to <p> to make the error go away.
return (
<div>
{data.dog.name} is a {data.dog.breed}
</div>
);
}
test.only("issue 11285", async () => {
const dogMock = {
request: {
query: GET_DOG_QUERY,
variables: { name: "Buck" }
},
result: {
data: { dog: { id: 1, name: "Buck", breed: "poodle" } }
}
};
render(
<MockedProvider mocks={[dogMock]} addTypename={false}>
<Dog name="Buck" />
</MockedProvider>
);
expect(await screen.findByText("Loading...")).toBeInTheDocument();
expect(await screen.findByText("Buck is a poodle")).toBeInTheDocument();
}); |
|
Ah, that makes sense! f766e83 changes from combining That is more in line with React's recommendations on how an external state as Apollo should be handling this, and was necessary to get the What you're seeing here is just that autobatching though - React 18 batches renders that appear close to each other and only renders the last of them. So your loading state is just never committed to the DOM. Don't ask my why it batches differently between If you add a slight delay before the result is delivered, the test behaves as expected: const dogMock = {
+ delay: 100,
request: {
query: GET_DOG_QUERY,
variables: { name: "Buck" }
},
result: {
data: { dog: { id: 1, name: "Buck", breed: "poodle" } }
}
}; |
|
Hah, nice! I can confirm this fixes the issue (the whole autobatching makes sense). TY @phryneas very much appreciated. This leaves the question how to do this correctly though. I've been playing around with setting |
|
I'd say that if you want to test a loading state, you have to make sure your app stays in loading state for long enough - set a Unfortunately, Testing Library only polls the DOM for changes, so there's always risk that a change might be missed or happen too fast for RTL to pick it up - so you have to make sure things are slow enough. |
|
The easiest way might be to separate loading state testing and loaded state testing. Maybe like so: it("should render dog loading", async () => {
const dogMock = {
request: {
query: GET_DOG_QUERY,
variables: { name: "Buck" }
},
delay: Infinity // load indefinitely
};
render(
<MockedProvider mocks={[dogMock]} addTypename={false}>
<Dog name="Buck" />
</MockedProvider>
);
expect(await screen.findByText("Loading...")).toBeInTheDocument();
});
it("should render dog loaded", async () => {
const dogMock = {
request: {
query: GET_DOG_QUERY,
variables: { name: "Buck" }
},
result: {
data: { dog: { id: 1, name: "Buck", breed: "poodle" } }
}
};
render(
<MockedProvider mocks={[dogMock]} addTypename={false}>
<Dog name="Buck" />
</MockedProvider>
);
expect(await screen.findByText("Buck is a poodle")).toBeInTheDocument();
});The If you think that is a viable solution, I can offer to:
WDYT? |
|
I think having a "shared" test is of interest for most users, so I'd say something like in the existing example would probably be best. As it stands, I don't think our own tests need any updates - they are working after all, but we are testing a lot of things using different paradigms. A PR for the docs there would be very welcome :) |
Following the discussion in apollographql#11285.
Fair enough. So folks can chose what works best for them. PR: #11287.
I more meant as a kind of "documentation test" to make sure the proposed way of testing things keeps working (especially the But at the end of the day, you are in a much better position to judge whether or not this is appropriate :) |
That is a good point - could you also add such a test to your PR? I think |
|
Done :) |
Looks like this is needed after the changes in For example, any test that tries to verify that |
Honestly: hard to say. 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. |
|
Thanks for the context @phryneas |
|
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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
Dogcomponent 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):
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
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:
The text was updated successfully, but these errors were encountered: