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 :)

[kamranayub]

I am still investigating an issue related to this and I've isolated it down to iteraction between two tests. When each one runs individually, they pass, but when run together, the second fails. If I call unmount in the first test, only then will the second pass. I am using @testing-library/react and rendering a component tree that uses multiple queries within it.

There seems to be cleanup that needs to happen besides just clearing cache (listeners?) and it doesn't seem to happen with RTL's auto-cleanup functionality. I'm hoping to reproduce minimally to figure out what requires an explicit unmount since this probably isn't ideal to require unmount in every test. 🤔

I plan to open an issue once I can reproduce in tests.

Update: This may be an issue with our usage and with queries used within a react-modal context, still investigating.

[kentcdodds]

Sounds like a race condition. Something goes wrong before the auto cleanup has a chance to run.

[kamranayub]

@kentcdodds OK, I figured it out! It's an issue related to the way react-query and react-modal cleanup on unmount. They both are using setTimeout calls when unmounting to set stale/cleanup/do garbage collection tasks.

The reason my tests pass in my suite if I use RTL's unmount explicitly is because when RTL then runs the cleanup function, it does this:

async function cleanup() {
  await flush()
  mountedContainers.forEach(cleanupAtContainer)
}

Notice the key here: it flushes the microtask queue. Since I call unmount before flushing the queue, everything works.

I went ahead and linked a one-line change to RTL and tested this new cleanup function:

async function cleanup() {
  mountedContainers.forEach(cleanupAtContainer)
  await flush()
}

Suddenly all my tests passed 😄 This runs the unmount first then flushes any remaining microtasks that may have been queued as part of the unmount process.

So now I can try to write a test case(s) to reproduce this within RTL itself and hopefully submit a PR, unless you feel this is enough information.

edit: I have a test case I'll submit in a PR.

test('cleanup waits for queued microtasks during unmount sequence', async () => {
  const spy = jest.fn()

  const Test = () => {
    React.useEffect(() => () => setImmediate(spy))

    return null
  }

  render(<Test />)
  await cleanup()
  expect(spy).toHaveBeenCalledTimes(1)
})

[kamranayub]

Update: I was able to isolate this to react-query only when manual: true is set in config and usage of prefetchQuery with force: true is set for multiple queries during a single test run.

I will open a draft PR to showcase it and then try to nail down a fix, if possible.

[Andreyco]

Was having same behaviour with useMutation hook.

Based on feedback found in this thread, I create custom factory for creating hook's wrapping component which tells Jest to clear query AND mutation cache after each test.

import React from 'react';
import {addCleanup} from '@testing-library/react-hooks';
import {
  MutationCache,
  QueryCache,
  QueryClient,
  QueryClientProvider,
} from 'react-query';

/**
 * Wrapper component providing Query context.
 */
export function createQueryWrapper() {
  const mutationCache = new MutationCache();

  const queryCache = new QueryCache();

  const queryClient = new QueryClient({
    queryCache,
    mutationCache,
    defaultOptions: {
      queries: {
        retry: false,
      },
    },
  });

  const wrapper: React.FC = (props) => {
    return (
      <QueryClientProvider client={queryClient}>
        {props.children}
      </QueryClientProvider>
    );
  };

  addCleanup(() => {
    mutationCache.clear();
    queryCache.clear();
  });

  return {
    queryClient,
    wrapper,
  };
}

[kopax]

Hi @Andreyco and thanks for offering an out of the box solution to those warning, I wanted to copy the code but I am using @testing-library/[email protected] and [email protected] and I don't have an import named addCleanup. What version do you use?

[Andreyco]

@kopax @testing-library/react-hooks typings are wrong - missing addCleanup signature.
As long as you ignore TS error (or extend typing to cover missing piece) you should be good.

Least you could do is

// @ts-expect-error
import {addCleanup} from '@testing-library/react-hooks';

[TkDodo]

why not just give every test their own QueryClien? this will get rid of the need for any cleanup. something like this:

export function useCustomHook() {
  return useQuery("customHook", () => "Hello");
}

const createWrapper = () => {
  const queryClient = new QueryClient();
  const wrapper = ({ children }) => (
    <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
  );

  return wrapper;
};

test("should work", async () => {
  const { result, waitFor } = renderHook(() => useCustomHook(), {
    wrapper: createWrapper()
  });

  await waitFor(() => result.current.isSuccess);

  expect(result.current.data).toEqual("Hello");
});

[Andreyco]

why not just give every test their own QueryClien? this will get rid of the need for any cleanup. something like this:

That is exactly what the snippet I posted does. If you care about query client in test, you have it exported. In addition, it does repetitive work for you 🤷‍♂️

[pducolin]

Is it expected to still see the act warning when not awaiting for the rendered hook to be successful with @testing-library/react-hooks? I have a codesandbox that reproduce it. I would have expected the queryCache.clear() to cancel all async requests.

Versions:

• [email protected]
• [email protected]
• [email protected]
• @testing-library/[email protected] <== updating to ^5.0.0 breaks the codesandbox, doesn't resolve the rendering function pure/js
• [email protected]