callstack / react-native-testing-library

🦉 Simple and complete React Native testing utilities that encourage good testing practices.
https://callstack.github.io/react-native-testing-library/
MIT License
3.05k stars 271 forks source link

Full example of how to use Act? #1348

Closed pipedreambomb closed 1 year ago

pipedreambomb commented 1 year ago

I'm sorry, but I am having a lot of trouble applying the advice in Understanding Act. Is there a full example somewhere of a whole test, including package imports, that applies these principles? I can't find one in the tests in this project or in the React Native Testing project.

It says "In the following tests we will directly use ReactTestRenderer instead of RNTL render function to render our component for tests." It doesn't explain if this is a necessary step, or if it's just creating an example. I assumed it's mandatory and I've tried copying that approach, but I get a bunch of error messages and I don't know what the hell I'm doing.

I just want a basic functional test that checks if my whole application loads up correctly. The code I'd be fine with would be something like this:

import React from 'react'
import { render, screen } from '@testing-library/react-native'
import { act } from 'react-dom/test-utils'

import App from './App'

describe('App', () => {
  it('should load', () => {
    // Act
    act(() => render(<App />))

    // Assert
    expect(screen.getByText('Round: 1')).toBeOnTheScreen()
  })
})

It outputs this in Jest:

FAIL  ./App.integration.test.jsx
  App
    ✕ should load (117 ms)

  ● App › should load

    Trying to detect host component names triggered the following error:

    Can't access .root on unmounted test renderer

    There seems to be an issue with your configuration that prevents React Native Testing Library from working correctly.
    Please check if you are using compatible versions of React Native and React Native Testing Library.

       8 |   it('should load', () => {
       9 |     // Act
    > 10 |     act(() => render(<App />))
         |                     ^
      11 |
      12 |     // Assert
      13 |     expect(screen.getByText('Round: 1')).toBeOnTheScreen()

      at detectHostComponentNames (node_modules/@testing-library/react-native/src/helpers/host-component-names.tsx:61:11)
      at configureHostComponentNamesIfNeeded (node_modules/@testing-library/react-native/src/helpers/host-component-names.tsx:26:30)
      at render (node_modules/@testing-library/react-native/src/render.tsx:40:38)
      at App.integration.test.jsx:10:21
      at act (node_modules/react/cjs/react.development.js:2510:16)
      at Object.<anonymous> (App.integration.test.jsx:10:8)

That doesn't work so I've tried to ape the Understanding Act doc and ended up with this (which still doesn't work):

import React from 'react'
import { within } from '@testing-library/react-native'
import { act } from 'react-dom/test-utils'
import TestRenderer from 'react-test-renderer'

import App from './App'

describe('App', () => {
  it('should load', () => {
    // Act
    let renderer
    act(() => {
      renderer = TestRenderer.create(<App />)
    })

    // Assert
    // Bind RNTL queries for root element.
    const view = within(renderer.root)

    expect(view.getByText('Round: 1')).toBeOnTheScreen()
  })
})

The output errors for this are so long I had to paste it in a Gist.

For background, my app is a game with a Redux data store and a bunch of things it does like populate the game board at the start. I'm not surprised it's a more complicated scenario than the basic examples of using this testing library that I can find. I just need to know how to proceed from here. Please!

mdjastrzebski commented 1 year ago

@pipedreambomb normally, you do not need to use act in tests. All of the relevant RNTL functions already use act inside them so you do not have to (render, update, fireEvent, etc).

So your example should look like:

import React from 'react'
import { render, screen } from '@testing-library/react-native'
import App from './App'

describe('App', () => {
  it('should load', () => {
    render(<App />);

    // Assert
    expect(screen.getByText('Round: 1')).toBeOnTheScreen()
  })
})

We have a basic example app that you can use as a reference for best practices.

pipedreambomb commented 1 year ago

Yeah, that's what I've done in all my other tests so far. But this one gave the warning when I ran it in Jest that I needed to use Act. Probably this one is different because the others were simple components and this is a smoke test for my entire application. I just can't figure out how 🙂

mdjastrzebski commented 1 year ago

You might need act in rare cases when you e.g. advance daje timers or otherwise cause external (to RNTL) change that might trigger React re-renders. However, all RNTL utils like render and fireEvent already use act internally.

pipedreambomb commented 1 year ago

Hi @mdjastrzebski, I tried running the test you suggested and I got the following output:

  console.error
    Warning: An update to App inside a test was not wrapped in act(...).

    When testing, code that causes React state updates should be wrapped into act(...):

    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */

    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act
        at App (/home/violentfemme/code/drop-bombs-mobile/App.jsx:25:25)

      at printWarning (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:72:30)
      at error (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:46:7)
      at warnIfUpdatesNotWrappedWithActDEV (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:16677:9)
      at scheduleUpdateOnFiber (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:14877:5)
      at setLoaded (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7510:7)
      at node_modules/@expo-google-fonts/josefin-sans/useFonts.js:21:19
      at tryCallOne (node_modules/promise/lib/core.js:37:12)

 FAIL  ./App.integration.test.jsx
  App
    ✕ should load (650 ms)

  ● App › should load

    Unable to find an element with text: Round: 1

       9 |
      10 |     // Assert
    > 11 |     expect(screen.getByText('Round: 1')).toBeOnTheScreen()
         |                   ^
      12 |   })
      13 | })
      14 |

      at Object.<anonymous> (App.integration.test.jsx:11:19)

  ● App › should load

    expect.assertions(1)

    Expected one assertion to be called but received zero assertion calls.

       5 |
       6 | describe('App', () => {
    >  7 |   it('should load', () => {
         |                           ^
       8 |     render(<App />)
       9 |
      10 |     // Assert

      at Object.<anonymous> (App.integration.test.jsx:7:27)

  ● App › should load

    expect.hasAssertions()

    Expected at least one assertion to be called but received none.

       5 |
       6 | describe('App', () => {
    >  7 |   it('should load', () => {
         |                           ^
       8 |     render(<App />)
       9 |
      10 |     // Assert

      at Object.<anonymous> (App.integration.test.jsx:7:27)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        4.519 s, estimated 5 s
Ran all test suites matching /App/i.
/home/violentfemme/code/drop-bombs-mobile/node_modules/redux-persist/lib/createPersistoid.js:98
    writePromise = storage.setItem(storageKey, serialize(stagedState)).catch(onWriteFail);
                                                                      ^

TypeError: Cannot read property 'catch' of undefined
    at writeStagedState (/home/violentfemme/code/drop-bombs-mobile/node_modules/redux-persist/lib/createPersistoid.js:98:71)
    at Timeout.processNextKey [as _onTimeout] (/home/violentfemme/code/drop-bombs-mobile/node_modules/redux-persist/lib/createPersistoid.js:87:7)
    at listOnTimeout (internal/timers.js:557:17)
    at processTimers (internal/timers.js:500:7)

What do you suggest? It seems to me like I need to use act, if I can find an example of how to do it.

mdjastrzebski commented 1 year ago

@pipedreambomb This seems to be an issue related to how your App component works. In order to diagnose the issue we need to be able to reproduce the issue. Pls create a minimal reproduction repo based on our basic example app, so that we can get better understanding of the issue.

In case you App component is using any async code, you should try to using async findBy* queries:

expect(await screen.findByText('Round: 1')).toBeOnTheScreen()
pipedreambomb commented 1 year ago

@mdjastrzebski I seem to have got the test working now, but I still get 8 of these very lengthy act() warnings. I think I've got to the point where I can live with just ignoring them.

I really don't think I can possibly create a minimal reproduction of it. I have 38 other tests using RNTL that don't have this problem in my project. It's only this test where I am testing the whole application, which uses timers and state changes as part of the game setup, where I get the issue. I think it's this complexity that causes the issue. I have no idea what a minimal version of that would look like.

pierrezimmermannbam commented 1 year ago

@pipedreambomb this issue is caused when you're testing asynchronous code or code with timers and some promises/timers aren't resolved before your test finishes. For instance, when testing a page that fetches some data, the way to fix this is to wait for the api calls to be finished by querying some text that would be displayed after the calls are done:

expect(await screen.findByText('myData')).toBeTruthy();

More generally the way to fix this is to wait for a visual indication that the asynchronous code has run.

You'll very rarely have to explicitly use act in your tests but very often it will mean that some code is ran after test is completed. While it can be ignored, it's better to fix it because it means that you're not testing the code that is ran once promises are resolved and it won't ressemble the way your users use your application.

Kent C.Dodds wrote a very good article on this explaining why it is problematic and how to fix it

pipedreambomb commented 1 year ago

Hi @pierrezimmermannbam, that's a great point. Thanks very much for the detailed explanation and link. Unfortunately I've already been down that path on my own, in the time since I posted my original question. My current working test (that still gets the act() warning) looks like this:

import { render, screen, waitFor } from '@testing-library/react-native'

import App from './App'
jest.mock('redux-persist', () => ({
  ...jest.requireActual('redux-persist'),
  persistReducer: jest.fn().mockImplementation((config, reducer) => reducer)
}))
jest.mock('redux-persist/integration/react', () => ({
  PersistGate: props => props.children
}))
jest.mock('@expo-google-fonts/josefin-sans', () => ({
  useFonts: () => ['just need this to return a non-empty array']
}))
jest.mock('expo-splash-screen')

describe('App', () => {
  it('should load', async () => {
    // Act
    render(<App />)

    // Assert
    await waitFor(() => {
      expect(screen.getByHintText('Current Round')).toBeOnTheScreen()
      expect(screen.getByHintText('Tiles Remaining')).toBeOnTheScreen()
      expect(screen.getByHintText('Next Tile')).toBeOnTheScreen()
      expect(screen.getAllByRole('button')).toHaveLength(8)
    })
  })
})

I've just tried your suggestion today, expect(await screen.ByHintText('Current Round')).toBeTruthy(), but I had the same results, i.e. act() warnings.

It seems like I can just give up on the issue now as I don't think I can replicate it for you guys to look at, nor is it a long-term problem for me. My finished app won't launch straight into the game and all its complexities, but open up a menu where you can choose New Game. Testing App.jsx will be much easier to test at that point, and I already have tests for my Game.jsx. I just really wanted an automated test in the meantime that would run before master branch merges and check if the whole thing still works.

mdjastrzebski commented 1 year ago

You should be able to refactor

// Assert
    await waitFor(() => {
      expect(screen.getByHintText('Current Round')).toBeOnTheScreen()
      expect(screen.getByHintText('Tiles Remaining')).toBeOnTheScreen()
      expect(screen.getByHintText('Next Tile')).toBeOnTheScreen()
      expect(screen.getAllByRole('button')).toHaveLength(8)
    })

into

// Assert

// Only need to wait for first async change if all the changes occur at the same time
// findBy = getBy + waitFor
expect(await screen.findByHintText('Current Round')).toBeOnTheScreen()

// Rest of the changes can be check syncronously, as they already happend during wait for the first change.
expect(screen.getByHintText('Tiles Remaining')).toBeOnTheScreen()
expect(screen.getByHintText('Next Tile')).toBeOnTheScreen()
expect(screen.getAllByRole('button')).toHaveLength(8)

You should not use more that on assertion inside waitFor (explanation: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#having-multiple-assertions-in-a-single-waitfor-callback)

Also use findBy* instead of getBy* + waitFor (explanation: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-waitfor-to-wait-for-elements-that-can-be-queried-with-find)

pipedreambomb commented 1 year ago

I can but it has the same issue with act() warnings when I run it. But I'll remember that technique for the future. thanks.

On Wed, 1 Mar 2023 at 16:16, Maciej Jastrzebski @.***> wrote:

You should be able to refactor

// Assert await waitFor(() => { expect(screen.getByHintText('Current Round')).toBeOnTheScreen() expect(screen.getByHintText('Tiles Remaining')).toBeOnTheScreen() expect(screen.getByHintText('Next Tile')).toBeOnTheScreen() expect(screen.getAllByRole('button')).toHaveLength(8) })

into

// Assert // Only need to wait for first async change if all the changes occur at the same time// findBy = getBy + waitForexpect(await screen.findByHintText('Current Round')).toBeOnTheScreen()expect(screen.getByHintText('Tiles Remaining')).toBeOnTheScreen()expect(screen.getByHintText('Next Tile')).toBeOnTheScreen()expect(screen.getAllByRole('button')).toHaveLength(8)

— Reply to this email directly, view it on GitHub https://github.com/callstack/react-native-testing-library/issues/1348#issuecomment-1450423122, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFFQW57XSZGZVOA5DPSASLWZ5Y7HANCNFSM6AAAAAAVHE7XWM . You are receiving this because you were mentioned.Message ID: @.***>

pierrezimmermannbam commented 1 year ago

Indeed no repro can really help for this issue and it can be quite tricky when testing large components such as the whole app. One way to debug this is to cut piece by piece the App component into a minimal reproduction so you can easily see what part of the application is updated. Ideally you want to write a test on the App when it does not contain much so that you can see if you don't have a warning on each addition, making it way easier to find the problem when it occurs for the first time. Good luck!

AugustinLF commented 1 year ago

To add to the last comments, a guideline I've wrote for the developers on the app I work on, is to try to understand what are all the async rendering that will happen, and to await for all of them, in as many await findBy* as needed.

Let say that in your example, Current Round will be shown after your API call /rounds load, and Tiles Remaining needs /tiles, you might want to refactor to:

expect(await screen.findByHintText('Current Round')).toBeOnTheScreen()
expect(await screen.findByHintText('Tiles Remaining')).toBeOnTheScreen()

// now that all your async calls have been resolved, it's safe to use `getBy*`
expect(screen.getByHintText('Next Tile')).toBeOnTheScreen()
expect(screen.getAllByRole('button')).toHaveLength(8)

A tricky case is that sometime when you write your tests (especially when using real timers in my experience), you can write it a first time and you only await the first async effect, while the second by coincidence actually resolved before the first one. This leads to some form of false confidence that you're actually waiting for everything, while it's not the case.

Another thing I've observed, is that sometimes you might have in the screen your rendering, other async effects, that you are not testing (because for instance they might be covered in other tests). Those will also trigger those types of warning. There you usually have two solutions: add awaits also for those effects, or mock them so they don't happen. Mocking part of the flow your testing is usually considered as a bad practice in the @testing-library ecosystem, but if it's something that you really don't care about, you might be able to safely ignore it.

pipedreambomb commented 1 year ago

Thanks for the great advice everyone. Very nice, detailed explanation from @AugustinLF at the end. I'll definitely be coming back to these comments the next time I'm struggling with similar issues. I'm going to close this thread now as I have stuff to move on to, and one day I won't need this exact test so I'll just focus on getting there instead :)

Edit: I might mention though that it's confusing that you have this big section of the wiki called Understanding Act function, but then every single one of you has advised me not to use it! Maybe you could put something on there about it being a last resort?