Recommendation on testing

[kentcdodds]

Hey there 👋

I'm getting an act warning when using react-query hooks because of this:

https://github.com/tannerlinsley/react-query/blob/df4bd3b54943db6acc4df612e6e85d6b09ea1a29/src/queryCache.js#L228-L237

Basically what's happening is my component finishes rendering and then react-query triggers a re-render (thanks to this) which is the thing that's giving me the act warning.

Now I can add an await waitFor(() => {}) call to the end of my test and the warning goes away, but I think it would be better to have an assertion in that callback. I'm just not sure what to assert happened as a result of the state update.

Do you have any suggestions for people testing this stuff? Should I just set the staleTime to Infinity? Kinda thinking that might work except then my tests would have a different config from my app which could lead to some problems.

Any ideas?

[tannerlinsley]

Right now it’s recommended to clear the query cache after each test, so what if the timeout or rerender checked to see if the query still exists in the cache or is mounted. Would that work?

…

On Mar 20, 2020, 1:40 PM -0600, Kent C. Dodds ***@***.***>, wrote: Hey there 👋 I'm getting an act warning when using react-query hooks because of this: https://github.com/tannerlinsley/react-query/blob/df4bd3b54943db6acc4df612e6e85d6b09ea1a29/src/queryCache.js#L228-L237 Basically what's happening is my component finishes rendering and then react-query triggers a re-render (thanks to this) which is the thing that's giving me the act warning. Now I can add an await waitFor(() => {}) call to the end of my test and the warning goes away, but I think it would be better to have an assertion in that callback. I'm just not sure what to assert happened as a result of the state update. Do you have any suggestions for people testing this stuff? Should I just set the staleTime to Infinity? Kinda thinking that might work except then my tests would have a different config from my app which could lead to some problems. Any ideas? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[kentcdodds]

what if the timeout or rerender checked to see if the query still exists in the cache or is mounted.

I think this would help!

[kentcdodds]

Yes, I just tried this and it worked well for me:

// added to queryCache code in `dispatch` function:
// right above this line: https://github.com/tannerlinsley/react-query/blob/df4bd3b54943db6acc4df612e6e85d6b09ea1a29/src/queryCache.js#L223

if (!queryCache.getQuery(query.queryKey)) {
  return
}

// in my test file:
afterEach(() => {
  queryCache.clear()
})

And that worked. However, I observed that if I put that afterEach in my setupTests.js file, it didn't clear the cache soon enough, so I'm thinking that upping the stale timeout a bit may help with that, though that seems like it could lead to some hacks, so what's probably better is to just put the queryCache.clear() call inside the test itself which does not sound like a lot of fun...

Either way, I think adding that if check to the dispatch function would be necessary.

I also did notice that the rerender call in onStateUpdate is already using useMountedCallback so that should be fine. We get the warning just when there's an unexpected setState update when the component is still mounted. So maybe if there's something we can assert so we can wrap that in a waitFor that would be better.

Still working through this...

[kentcdodds]

Question... Why are we triggering a re-render anyway?

[tannerlinsley]

It uses a redux style subscription architecture. The render reconciles new mutable state changes into the ui. We may not need this in the future with useMutableSource

…

On Mar 20, 2020, 4:13 PM -0600, Kent C. Dodds ***@***.***>, wrote: Question... Why are we triggering a re-render anyway? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[kentcdodds]

Whoops, didn't mean to close

[kentcdodds]

I thought it was something like that...

I'm just trying to think of whether there's anything I can assert so I can put that within the waitFor(() => {/* expect right here */}) but there's nothing observable in the DOM that changes here and I can't think of anything in the query cache I could assert either... Hmmm....

[tannerlinsley]

Yeah the idea of a stale query is not a ui bound variable by any means.

…

On Mar 20, 2020, 4:18 PM -0600, Kent C. Dodds ***@***.***>, wrote: I thought it was something like that... I'm just trying to think of whether there's anything I can assert so I can put that within the waitFor(() => {/* expect right here */}) but there's nothing observable in the DOM that changes here and I can't think of anything in the query cache I could assert either... Hmmm.... — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[kentcdodds]

I'm making a PR for the dispatch thing right now :)