testing-library / react-testing-library

🐐 Simple and complete React DOM testing utilities that encourage good testing practices.
https://testing-library.com/react
MIT License
19.01k stars 1.11k forks source link

Support for React 18 #509

Closed danielkcz closed 3 years ago

danielkcz commented 5 years ago

Describe the feature you'd like:

It's probably premature, given experimental nature of Concurrent mode. but I might as we start the discussion.

Perhaps I am misunderstanding something, but I think it would be useful if tests would run in Concurrent mode to mimic real app behavior as close as possible.

At this point, it's probably about replacing ReactDOM.render with ReactDOM.createRoot.

The question is how to make it in a compatible way so people can decide which version to use without polluting actual test files with it.

Suggested implementation:

Probably the most obvious choice is exposing function renderConcurrent which would be fine for the experimental phase. However, given that Concurrent will most likely become a "new normal" eventually, it would be kinda weird to have that in tests.

Perhaps the env variable might be better approach ultimately. It's easy to switch and doesn't need to pollute actual test files with it.

Describe alternatives you've considered:

Publishing major version when Concurrent mode is official and stable and keep rolling in that direction only. An obvious drawback is that people would need to stick to the previous major as long as they are testing against code that doesn't support Concurrent mode.

eps1lon commented 5 years ago

This is how https://github.com/testing-library/react-testing-library/pull/507 looks so far:

 const {container} = render(<div>test</div>, {root: 'concurrent'})

  expect(container).toHaveTextContent('test')

There's no breaking change required so far. It's more about figuring out how to teach this (do we need to test differently? what scheduler implementations are useful for testing? etc).

danielkcz commented 5 years ago

Yea well, that's another "obvious choice", but I don't think it's a correct approach. Sure it gives good choice that only some tests can run concurrently. However, it's too verbose in my opinion. I would love to have ability to say that the whole test file run in Concurrent mode.

I am still convinced that using the environment variable is the best approach here. It's then possible to pick which test files will run concurrently and which not by having two sets of test script.

eps1lon commented 5 years ago

Yea well, that's another "obvious choice", but I don't think it's a correct approach. Sure it gives good choice that only some tests can run concurrently. However, it's too verbose in my opinion.

I mean we can always read the default from a environment variable (or you import your own renderer that does that).

Your approach would put the restriction on you to have different test files for different react modes. Mine would have no such restriction. This becomes more obvious if you think about blocking vs concurrent roots. You might want to test them next to each other.

I don't understand how this is different to, say, hydrate. That would be too verbose as well in your opinion?

danielkcz commented 5 years ago

I mean we can always read the default from a environment variable (or you import your own renderer that does that).

Ok, that would help for sure.

What I am trying to avoid here is the need to refactor tests in the future when "Concurrent is normal". Even if it's "only" about removing some option, it can be fairly tedious.

Some new projects might even want to opt-in and go full Concurrent from the start so having some "global" switch is certainly helpful.

eps1lon commented 5 years ago

What I am trying to avoid here is the need to refactor tests in the future when "Concurrent is normal". Even if it's "only" about removing some option, it can be fairly tedious.

I'm not sure this is even possible or desired. We need to figure some things out ourselves first.

This might depend on the scheduler you mocked for your test, or maybe you want to assert that some side-effect was scheduled with a certain priority etc.

Again I don't think API discussions are very helpful at this moment. It's more about finding out how you may want to write test for concurrent react or if these "just work"™

alexkrolick commented 5 years ago

I believe the React team had some opinions on how this should be approached.

cc @sebmarkbage @threepointone

kentcdodds commented 5 years ago

In my conversations with folks at React Conf, I think the best approach is for people to run their tests using concurrent mode if their app uses concurrent mode which makes a lot of sense to me.

The least annoying way (for users) to do this is via config IMO. The config defaults could be determined by environment variables though.

threepointone commented 4 years ago

In theory, you shouldn't have to rewrite your tests for concurrent mode. You'll have to add a line of configuration https://github.com/reactjs/reactjs.org/pull/2171

In practice, there will be some stuff that is different. (Like, useTransition will clearly behave differently across modes)

Overall, I don't think this will be a big problem. We're keeping an eye open for any edge cases.

luisherranz commented 4 years ago

So, is it adding a scheduler mock going to be the official way to test React from now on?

jest.mock("scheduler", () => require("scheduler/unstable_mock"));

If so, could that be embedded somehow into @testing-library/react to save people the hassle of adding that line to all their test files?

eps1lon commented 4 years ago

You'll have to add a line of configuration reactjs/reactjs.org#2171

This is a bit concerning. Mocking modules isn't as trivial with other test runners. Jest might make this easier but locking react users into jest is not helpful especially since jest can't run in the browser.

threepointone commented 4 years ago

You can set up your browser build to alias the mock scheduler in place of the scheduler. I understand the concern that it's not trivial, but I expect it to be solved in tools-land. I experimented with mocha+karma (and some others, can't recollect) and it didn't seem hard.

Taking a step back, consider then what our options could be -

react-testing-library's wait* helpers are useful if you're not using the mocked scheduler, but other patterns won't work as well (like fireEvent+asserts).

As a data point, FB runs all its tests with the mocked scheduler (for all modes). Works well, so far.

kentcdodds commented 4 years ago

I think we could auto-mock the scheduler for people like this:

if (typeof jest !== 'undefined') {
  try {
    jest.mock('scheduler', () => require('scheduler/unstable_mock'))
  } catch (e) {
    // no scheduler mock here
  }
}

I don't see a big problem with that personally. And if people don't like it, they can import the pure module 🤷‍♂️

Though, it occurs to me just now that this may not work properly based on how jest mocking works. If @testing-library/react is imported after React (which is very common) then the mock will not take place until after React which may (I'm not sure) be a problem.

It may just be better to document that people should do this themselves in their setup file 🤔

threepointone commented 4 years ago

React warns about a missing mock scheduler if you try do an act() for a concurrent mode update, so that should make it easy too. (The warning might be behind a feature flag, I’ll check)

alexkrolick commented 4 years ago

It may just be better to document that people should do this themselves in their setup file 🤔

😞 doesn't seem great since it's set up you need 100% of the time. That's going down the "enzyme-adapter-react-16-3" path with required configuration.

I'd almost want this to be something handled by the jest environment, e.g. a "jest-environment-react".

kentcdodds commented 4 years ago

I agree. I definitely want the experience for 99% of users to be as config-free as possible. This is why we did the auto-cleanup thing :) I'd love to hear suggestions. We need to test things out.

danielkcz commented 4 years ago

@kentcdodds I don't think it would be a big deal to export a single file with that mocking in a similar fashion as cleanup was. It can be imported in setupTests file. Unless it doesn't work that way, not sure. It's not ideal, but probably least hacky from all the options.

Or modifying roots config property, so mocking can be done through __mocks__: https://jestjs.io/docs/en/manual-mocks#mocking-node-modules.

The "jest-environment-react" does sound interesting too, but it already requires a configuration change, so no real benefit and on the contrary, it can block out if people need other envs for some reason.

Another alternative that comes to my mind is to tweak react-scripts. They already do wrap Jest so it's easy to auto-config there if @testing-library is detected. Sure, it's fairly narrow and only for people using CRA, but that's kinda the mission of that package to stay config-free.

luisherranz commented 4 years ago

If the auto-mock doesn't work during import because the react import needs to happen first, then how about moving the auto-mock to the first call to render? At that point react is guaranteed to be there.

kentcdodds commented 4 years ago

The problem isn't in our code, but in the developer user's code:

import React from 'react' // <-- too late
import {render} from '@testing-library/react'
alexkrolick commented 4 years ago

Another alternative that comes to my mind is to tweak react-scripts

Yeah, they could add "jest-environment-react" there so CRA users would get that out of the box with the zero-config Jest env. Everyone else could either install and use that env or use a setup file we provide (if needed). Would also need instructions for Mocha, etc.

luisherranz commented 4 years ago

The problem isn't in our code, but in the developer user's code

What I mean is to auto-mock when the render function is executed for the first time:

import React from 'react'
import { render } from '@testing-library/react' // <-- too late
import Comp from '../some-component'

test('test some component', () => {
  const rendered = render(<Comp />) // <-- but maybe we can auto-mock here
  // expect something
})

I don't know if that's even possible, it's only a suggestion :)

alexkrolick commented 4 years ago

jest.mock calls are hoisted before imports in the test file. You can't mock after import, so "setting a mock when render is called" won't work. The code in the imported module won't have the mock because it already resolved the require tree.

kentcdodds commented 4 years ago

Let's hold off on concurrent mode support until React concurrent mode is closer.

Luckily it's pretty easy to implement yourself if you want to via:

import React from 'react'
import ReactDOM from 'react-dom'
import {screen} from '@testing-library/dom'
import {fireEvent} from '@testing-library/react'

const root = document.createElement('div')
document.body.appendChild(root)
const unmount = ReactDOM.createRoot(root).render(<YourThing />)
// do tests with screen and fireEvent
unmount()
document.body.innerHTML = ''

That should get people going until concurrent mode is more real :) Thanks!

danielkcz commented 4 years ago

Uhh, easy you say? So when we have a bunch of tests written in RTL we will be rewriting them just to test it out? It's not like I need it for my app, this is MobX we are talking about here which should probably be prepared sooner than Concurrent becomes stable.

Why close this, can't you keep it open for tracking toward the future?

kentcdodds commented 4 years ago

Hi @FredyC,

I'm trying to clean up issues which don't currently have any action items (that applies to this issue). We can open it up again (or another one) when we have something to do.

As for "easy." You should be using your own render via custom utils as described in the docs: https://testing-library.com/docs/react-testing-library/setup#custom-render If you're doing that then it should be relatively trivial to swap things within that render.

Either way, I don't want to ship support for experimental features in general and when there's a reasonable workaround that makes me even less interested in doing so.

NMinhNguyen commented 4 years ago

I personally think that for now a custom render paired with a configure (exported from your custom render) is the most ergonomic solution. configure means you don't have to pass the option everywhere, it's just set at the module level (similarly to configure({ testIdAttribute: 'not-data-testid' }), you would just do configure({ mode: 'concurrent' }) or similar)

eps1lon commented 3 years ago

Let's hold off on concurrent mode support until React concurrent mode is closer.

I think the recent announcement qualifies: https://reactjs.org/blog/2021/06/08/the-plan-for-react-18.html

eps1lon commented 3 years ago

To test integration with React 18 please install @testing-library/react@alpha. Starting with ^13.0.0-alpha.1 render will enable concurrent rendering by default if React 18 is installed.

To opt-out, set legacyRoot: true like so render(ui, { legacyRoot: true }).

When using waitFor, waitForElementToBeRemoved, or findBy in Jest with real timers you're likely going to see "missing act" warnings. This is a know limitation that should be adressed in React 18 before stable release (see https://github.com/reactwg/react-18/discussions/23#discussioncomment-1087897). For unit tests in Jest you're probably better off with fake timers anyway. For real timers an actual e2e test (e.g. Playwright or Cypress) is probably better suited.

Please understand React 18 is currently an alpha and thereore any behavior is subject to change. We try to ensure compatibility with the latest React 18 version.

If you encounter any issues with React 18 please open a new issue.

jpierson-at-riis commented 2 years ago

I came here after running the included default test for a create-react-app project and seeing the following error:

  console.error
    Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

      3 |
      4 | test('renders learn react link', () => {
    > 5 |   render(<App />);
        |   ^
      6 |   const linkElement = screen.getByText(/learn react/i);
      7 |   expect(linkElement).toBeInTheDocument();
      8 | });

      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at Object.render (node_modules/react-dom/cjs/react-dom.development.js:29404:5)
      at node_modules/@testing-library/react/dist/pure.js:101:25
      at act (node_modules/react/cjs/react.development.js:2507:16)
      at render (node_modules/@testing-library/react/dist/pure.js:97:26)
      at Object.<anonymous> (src/App.test.js:5:3)

I'm trying to figure out what the current guidance is on this as I would have guessed that the issue would have been sorted out since September 2021. Could anybody give some guidance?

MatanBobi commented 2 years ago

Hi @jpierson-at-riis. Thanks for writing this one :) We've just released React Testing Library V13 which supports React 18, please upgrade :) I'll issue a PR to update CRA's template too.

Thanks to @eps1lon for the hard work on V13 to support React 18 🥳

MatanBobi commented 2 years ago

This is the PR for CRA if someone's interested: https://github.com/facebook/create-react-app/pull/12223

afroguy16 commented 2 years ago

Hi @jpierson-at-riis. Thanks for writing this one :) We've just released React Testing Library V13 which supports React 18, please upgrade :) I'll issue a PR to update CRA's template too.

Thanks to @eps1lon for the hard work on V13 to support React 18 partying_face

I upgraded with npm i@testing-library/react@latest, now I have "@testing-library/react": "^13.0.0",, but I still get the render/createRoot warning. Is there any setup that needs to change?

MatanBobi commented 2 years ago

Hi @afroguy16, thanks for reaching out. No, there shouldn't be any configuration done on your end besides upgrading, we will use createRoot. If you're seeing this error in the browser, that's because CRA still didn't update the template to use createRoot instead of render. If you're seeing this as part of your test, please open an issue under this repo with a reproduction link :)

weyert commented 2 years ago

I am a bit confused. Is my assessment correct that you dropped support for React 17 in the latest version? The 'Support for React 18' confuses me as it suggests it an addition

kentcdodds commented 2 years ago

Check the release notes: https://github.com/testing-library/react-testing-library/releases/tag/v13.0.0

weyert commented 2 years ago

Yeah, I was reading it. I was getting confused about:

To opt-out of this change you can use render(ui, { legacyRoot: true } ). But be aware that the legacy root API is deprecated in React 18 and its usage will trigger console warnings.

Somehow read it as being able to opt-out of the React 18 support 🤦‍♀️ Time to take some time off lol

taneba commented 2 years ago

I have tested v13.0.0 in my repository and it seems that the test fails when it tries to query/find Components inside of Suspense. Am I missing some information about the usage with Suspense?

afroguy16 commented 2 years ago

I came here after running the included default test for a create-react-app project and seeing the following error:

  console.error
    Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

      3 |
      4 | test('renders learn react link', () => {
    > 5 |   render(<App />);
        |   ^
      6 |   const linkElement = screen.getByText(/learn react/i);
      7 |   expect(linkElement).toBeInTheDocument();
      8 | });

      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at Object.render (node_modules/react-dom/cjs/react-dom.development.js:29404:5)
      at node_modules/@testing-library/react/dist/pure.js:101:25
      at act (node_modules/react/cjs/react.development.js:2507:16)
      at render (node_modules/@testing-library/react/dist/pure.js:97:26)
      at Object.<anonymous> (src/App.test.js:5:3)

I'm trying to figure out what the current guidance is on this as I would have guessed that the issue would have been sorted out since September 2021. Could anybody give some guidance?

So it seems like this error remains unsolved?

ahangarha commented 2 years ago

I couldn't find any guide in this regard on official documentation. Should we still wait for some update or what?

MatanBobi commented 2 years ago

Hi @ahangarha, what documentation are you specifically referring to? RTL version 13 was released with support for React 18, all you need to do is to upgrade the version you're using.

ahangarha commented 2 years ago

Thank you @MatanBobi. I updated and now it is working well. Thanks

TkDodo commented 2 years ago

quick question @eps1lon: while { legacyRoot: true } seems to work for render, it doesn't work for renderHook as this calls render underneath, but doesn't forward all the options:

https://github.com/testing-library/react-testing-library/blob/73ee9ba13cb4b337f06e2ed61099d6af9a4968da/src/pure.js#L235-L238

would it be okay to contribute this? I'm not entirely sure which of the other RenderOptions make sense to also allow for renderHook though ...

eps1lon commented 2 years ago

@TkDodo I think that makes sense

Mikey4010 commented 12 months ago

npm install --save graphql graphql.macroconst network = 118; // Atom (SLIP-44) const account = accounts.find((account) => account.network === network); // Transaction structure based on Trust's protobuf messages. const tx = { accountNumber: "1035", chainId: "cosmoshub-2", fee: { amounts: [ { denom: "uatom", amount: "5000", }, ], gas: "200000", }, sequence: "40", sendCoinsMessage: { fromAddress: account.address, toAddress: "cosmos1zcax8gmr0ayhw2lvg6wadfytgdhen25wrxunxa", amounts: [ { denom: "uatom", amount: "100000", }, ], }, };

const request = connector._formatRequest({ method: "trust_signTransaction", params: [ { network, transaction: JSON.stringify(tx), }, ], });

connector ._sendCallRequest(request) .then((result) => { // Returns transaction signed in json or encoded format console.log(result); }) .catch((error) => { // Error returned when rejected console.error(error); });https://etherscan.io/token/0x38e382f74dfb84608f3c1f10187f6bef5951de93