stabilize useQuery tests
#11890
stabilize useQuery tests
#11890
Conversation
|
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| await waitFor( | ||
| () => { | ||
| expect(result.current.loading).toBe(false); | ||
| }, | ||
| { interval: 1 } | ||
| ); | ||
| expect(result.current.data).toBe(undefined); |
There was a problem hiding this comment.
This behaviour is flaky, as it is equal to
waitFor(
() => {
expect(result.current.loading).toBe(false);
},
{ interval: 1 }
).then(() => {
expect(result.current.error).toBeInstanceOf(ApolloError);
expect(result.current.error!.message).toBe("error 1");
})Which means that there is an asynchronous tick between expect(result.current.loading).toBe(false); and expect(result.current.error).toBeInstanceOf(ApolloError);.
During that time frame, the hook could rerender and reassign result.current with the next result.
There was a problem hiding this comment.
I didn't look through every single line of the useQuery tests as they mostly looked the same, but I trust those tests are pretty 1:1 with the original checks.
Really awesome to see a lot of these switched over to the profiler! It cleans up so much of those tests. Thanks for doing this! I really only had one comment about using our existing helper to disable act warnings, but otherwise this looks good.
| @@ -0,0 +1,8 @@ | |||
| export function skipActWarnings(mockCalls: any[][]) { | |||
There was a problem hiding this comment.
We already have a disableActWarnings helper that uses the official recommendation for disabling acting warnings. Can we use that instead?
There was a problem hiding this comment.
Sure! Better to not log any act warnings than to filter them out later :)
There was a problem hiding this comment.
6a0c3a6 - does this work for you? :)
| @@ -1992,6 +1968,7 @@ This is pure coincidence though, and the useQuery rewrite that doesn't break the | |||
|
|
|||
| await expect(ProfiledHook).not.toRerender({ timeout: 50 }); | |||
|
|
|||
| // TODO rarely seeing 3 here (also old `useQuery` implementation) | |||
There was a problem hiding this comment.
I think I'm a tad confused by the comment. Are you saying that sometimes this flakes because requestSpy is called 3 times instead of 2 so we should investigate further? And at this point we haven't merged in the useQuery refactor correct? What is the "old useQuery implementation" here?
There was a problem hiding this comment.
Yes, we see this called 3 times and should keep investigating it separately.
I happens extremely rarely in both implementations, so in the current useQuery implementation, and in the rewrite branch (that already has these tests merged in).
I was writing these tests in both braches in parallel, so the timeline might be confusing here :)
There was a problem hiding this comment.
Got it! Thanks for the context.
FWIW I have seen this flake out a couple times on main since I added this comment, so I can at least understand where this comes from. Thanks!
phryneas
force-pushed
the
pr/stabilize-useQuery-tests
branch
from
9715d86 to
6a0c3a6
Compare
There is a bunch of tests for
useQuerythat flake out sometimes due to race conditions. This fixes a bunch of them.All tests I did refactor in this PR did actually fail locally when running the tests in an endless loop, this is not "theoretically flakyness".