build: adjust enzyme, RTL and jest to run in React 18 runtime #6940

tkajtoch · 2023-07-12T21:55:42Z

Summary

This PR adds @cfaester/enzyme-adapter-react-18 and conditionally switches between React 17 and React 18 adapters depending on the REACT_VERSION environment variable. It also converts jest config file to JS and configures jest mappers to switch react, react-dom and @testing-library/react module versions depending on REACT_VERSION.

Jest tests are using the new environment variable to switch between tested React versions. For now it's required to manually set the REACT_VERSION environment variable if you want to use a non-default version. It's different from the recently added cypress script argument (--react-version) but will be unified in another PR. That's why I decided to not document setting REACT_VERSION for yarn test-unit right now.

QA

Please note that CI will fail for this PR due to some tests and typings not being compatible with React 18 yet.

Checkout this branch
Run yarn
Run REACT_VERSION=17 yarn test-unit and see all tests passing
Run yarn test-unit and see many tests failing

…t when REACT_VERSION is 18

…on for testing React 16 and 17

…me adapter

tkajtoch · 2023-07-12T21:56:28Z

scripts/jest/config.js

+  require('jest-config/build/getCacheDirectory').default;
+
+// Set REACT_VERSION env variable to latest if empty or invalid
+if (!['16', '17', '18'].includes(process.env.REACT_VERSION)) {


We could replace this with the utility function but Jest 24 doesn't support TypeScript config files

tkajtoch · 2023-07-12T21:56:55Z

scripts/jest/config.js

+
+const reactVersion = process.env.REACT_VERSION;
+
+console.log(`Running tests on React v${reactVersion}`);


I don't like this but hopefully we can move it to the test script when available :)

I'm kind of okay with it here, but yeah, having it in the individual test scripts would be helpful for thrown errors.

I think I may need to take a step back here to ask what our goal is with running our test suites across multiple versions of Kibana. 🤔 I thought we were doing it for Cypress as a very basic smoke test that rendering worked, especially for portals etc. which were most affected by the React 18 upgrade.

Running all our jest suites on multiple versions is going to fairly significantly impact our CI times. What are our plans to mitigate that, and what precisely are our goals for those tests?

edit: Just want to clarify the above is not a change request or a blocker, I just want to make sure we've thought this through!

We publish our library as compatible with React 16, 17 and (soon) 18. We should make sure it works and will continue to work with all changes we make no matter what EUI-compatible React version you're using.

The time to run all unit and e2e tests on multiple versions of react scales linearly, but that's okay. @1Copenut and I already discussed ways to parallelize it, so the total time to run tests should equal the longest single (unit or e2e) test run. Of course that may differ when the number of resources is limited due to, for example, building multiple PR environments at the same time. In this case we will prioritize builds on main.

👍 Gotcha, thank you for explaining! Parallelization makes total sense. One other option could also be to run non-React18 tests (i.e., older versions that we "support" but aren't first-class citizens) on a daily/night cron job instead of per-PR. I'm not sure if that makes more sense than parallelization though - do you have a preference?

The nice thing here, we can do pretty much whichever we want. Thinking out loud a second, breaking these into parallel runs shouldn't be too difficult in Builldkite. Instead of running a consolidated test command, we can set up the job with steps and multiple commands that each pass a version flag. We can increase resources if we exhaust the agent, or if the time isn't to our liking, def. run the older version tests on a cron.

tkajtoch · 2023-07-12T21:58:16Z

src/components/form/form.styles.test.tsx

      "
          padding: 7px;
-
+          


I think it comes from different whitespace serializing in the latest @testing-library/react?

1Copenut

👍 LGTM! I appreciate the clear explanation in your PR description for what's happening and why you've opted not to document the change yet. The upcoming work to unify how React versions are passed will be a great opportunity to make consistent and document.

1Copenut · 2023-07-13T15:29:34Z

scripts/jest/config.js

+
+const reactVersion = process.env.REACT_VERSION;
+
+console.log(`Running tests on React v${reactVersion}`);


I'm kind of okay with it here, but yeah, having it in the individual test scripts would be helpful for thrown errors.

cee-chen · 2023-07-13T18:04:24Z

scripts/jest/setup/polyfills.js

+Object.defineProperty(global, 'TextEncoder', {
+  value: util.TextEncoder,
+});


@tkajtoch Can you add a comment explaining why this polyfill had to be added?

Sure, I'll do that in the code!

cee-chen · 2023-07-13T18:05:37Z

src/test/index.ts

 } from './react_warnings';
 export { sleep } from './sleep';
 export * from './emotion-prefix';
+export * from './react_version';


I have some slight concerns here. Why do we want to make these utilities publicly available to consumers?

And if we don't, can we please move this to src/test/internal/ instead?

This definitely shouldn't be publicly available and won't even work when used outside test environment. I honestly thought none of src/test things are included in the build, but I just noticed that's not the case. I'll move it to src/test/internal in a new PR!

Perfect, thanks Tomasz!

* build: convert jest config to .js * build: add react version switching * build: add @cfaester/enzyme-adapter-react-18 enzyme adapter and use it when REACT_VERSION is 18 * build: update @testing-library/react and provide an alternative version for testing React 16 and 17 * build: polyfill TextEncoder for running React 18 tests * build: set IS_REACT_ACT_ENVIRONMENT = true when running React 18 enzyme adapter * test: reset dnd server context when running on react 16 and 17 * test: fix fireEvent import to use act-wrapped react equivalent * test: update form inline snapshot due to whitespace mismatch

tkajtoch added 9 commits July 12, 2023 23:19

build: convert jest config to .js

e87a49b

build: add react version switching

5a21b5c

build: add @cfaester/enzyme-adapter-react-18 enzyme adapter and use i…

69ab23a

…t when REACT_VERSION is 18

build: update @testing-library/react and provide an alternative versi…

a7381f9

…on for testing React 16 and 17

build: polyfill TextEncoder for running React 18 tests

be87a58

build: set IS_REACT_ACT_ENVIRONMENT = true when running React 18 enzy…

dd05514

…me adapter

test: reset dnd server context when running on react 16 and 17

b25b120

test: fix fireEvent import to use act-wrapped react equivalent

5826c50

test: update form inline snapshot due to whitespace mismatch

da98004

tkajtoch requested a review from a team July 12, 2023 21:55

tkajtoch self-assigned this Jul 12, 2023

tkajtoch commented Jul 12, 2023

View reviewed changes

tkajtoch mentioned this pull request Jul 13, 2023

test: React 18 act() adjustments #6943

Merged

1Copenut approved these changes Jul 13, 2023

View reviewed changes

tkajtoch merged commit 212ac52 into elastic:feature/react-18 Jul 13, 2023

cee-chen reviewed Jul 13, 2023

View reviewed changes


		const reactVersion = process.env.REACT_VERSION;

		console.log(`Running tests on React v${reactVersion}`);

build: adjust enzyme, RTL and jest to run in React 18 runtime #6940

build: adjust enzyme, RTL and jest to run in React 18 runtime #6940

Uh oh!

Conversation

tkajtoch commented Jul 12, 2023

Summary

QA

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cee-chen Jul 13, 2023 •

edited

Loading

cee-chen Jul 13, 2023 •

edited

Loading

cee-chen Jul 13, 2023 •

edited

Loading