-
Notifications
You must be signed in to change notification settings - Fork 861
build: adjust enzyme, RTL and jest to run in React 18 runtime #6940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e87a49b
5a21b5c
69ab23a
a7381f9
be87a58
dd05514
b25b120
5826c50
da98004
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| const getCacheDirectory = | ||
| 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)) { | ||
| process.env.REACT_VERSION = '18'; | ||
| } | ||
|
|
||
| const reactVersion = process.env.REACT_VERSION; | ||
|
|
||
| console.log(`Running tests on React v${reactVersion}`); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this but hopefully we can move it to the test script when available :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm kind of okay with it here, but yeah, having it in the individual test scripts would be helpful for thrown errors.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| /** @type {import('jest').Config} */ | ||
| const config = { | ||
| rootDir: '../../', | ||
| roots: [ | ||
| '<rootDir>/src/', | ||
| '<rootDir>/src-docs/src/components', | ||
| '<rootDir>/scripts/babel', | ||
| '<rootDir>/scripts/tests', | ||
| '<rootDir>/scripts/eslint-plugin', | ||
| ], | ||
| collectCoverageFrom: [ | ||
| 'src/{components,services,global_styling}/**/*.{ts,tsx,js,jsx}', | ||
| '!src/{components,services,global_styling}/**/*.{testenv,spec,a11y,stories}.{ts,tsx,js,jsx}', | ||
| '!src/{components,services,global_styling}/index.ts', | ||
| '!src/{components,services,global_styling}/**/*/index.ts', | ||
| '!src/components/date_picker/react-datepicker/**/*.{js,jsx}', | ||
| ], | ||
| moduleNameMapper: { | ||
| '\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': | ||
| '<rootDir>/scripts/jest/mocks/file_mock.js', | ||
| '\\.(css|less|scss)$': '<rootDir>/scripts/jest/mocks/style_mock.js', | ||
| }, | ||
| setupFiles: [ | ||
| '<rootDir>/scripts/jest/setup/enzyme.js', | ||
| '<rootDir>/scripts/jest/setup/throw_on_console_error.js', | ||
| '<rootDir>/scripts/jest/setup/mocks.js', | ||
| ], | ||
| setupFilesAfterEnv: [ | ||
| '<rootDir>/scripts/jest/setup/polyfills.js', | ||
| '<rootDir>/scripts/jest/setup/unmount_enzyme.js', | ||
| ], | ||
| coverageDirectory: '<rootDir>/reports/jest-coverage', | ||
| coverageReporters: ['json', 'html'], | ||
| moduleFileExtensions: ['ts', 'tsx', 'js', 'json'], | ||
| testMatch: ['**/*.test.js', '**/*.test.ts', '**/*.test.tsx'], | ||
| transform: { | ||
| '^.+\\.(js|tsx?)$': 'babel-jest', | ||
| }, | ||
| snapshotSerializers: [ | ||
| '<rootDir>/node_modules/enzyme-to-json/serializer', | ||
| '<rootDir>/scripts/jest/setup/emotion', | ||
| ], | ||
| // react version and user permissions aware cache directory | ||
| cacheDirectory: `${getCacheDirectory()}_react-${reactVersion}`, | ||
| }; | ||
|
|
||
| if (['16', '17'].includes(reactVersion)) { | ||
| config.moduleNameMapper[ | ||
| '^@testing-library/react((\\\\/.*)?)$' | ||
| ] = `@testing-library/react-16-17$1`; | ||
| config.moduleNameMapper['^react((\\/.*)?)$'] = `react-${reactVersion}$1`; | ||
| config.moduleNameMapper[ | ||
| '^react-dom((\\/.*)?)$' | ||
| ] = `react-dom-${reactVersion}$1`; | ||
| } | ||
|
|
||
| module.exports = config; | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,18 @@ | ||
| import { configure } from 'enzyme'; | ||
| import Adapter from '@wojtekmaj/enzyme-adapter-react-17'; | ||
|
|
||
| // Pick Enzyme adapter based on which React version is currently being tested. | ||
| // It has to be directly compared against process.env.REACT_VERSION | ||
| // for tree-shaking to work. | ||
| let Adapter; | ||
| if (process.env.REACT_VERSION === '18') { | ||
| Adapter = require('@cfaester/enzyme-adapter-react-18').default; | ||
|
|
||
| // set IS_REACT_ACT_ENVIRONMENT to silence "The current testing environment | ||
| // is not configured to support act()" errors for now | ||
| // TODO: Remove when enzyme tests are replaced with RTL | ||
| global.IS_REACT_ACT_ENVIRONMENT = true; | ||
| } else { | ||
| Adapter = require('@wojtekmaj/enzyme-adapter-react-17'); | ||
| } | ||
|
|
||
| configure({ adapter: new Adapter() }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| import { MutationObserver, MutationNotifier } from '../polyfills/mutation_observer'; | ||
| import util from 'util'; | ||
|
|
||
| // polyfill window.MutationObserver and intersect jsdom's relevant methods | ||
| // from https://github.com/aurelia/pal-nodejs | ||
| // https://github.com/aurelia/pal-nodejs/blob/56396ff7c7693669dbafc8e9e49ee6bc29472f12/src/nodejs-pal-builder.ts#L63 | ||
|
|
||
| Object.defineProperty(global, 'TextEncoder', { | ||
| value: util.TextEncoder, | ||
| }); | ||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tkajtoch Can you add a comment explaining why this polyfill had to be added?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll do that in the code! |
||
|
|
||
| const undos = []; | ||
| afterAll(() => { | ||
| while (undos.length) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,7 +181,7 @@ describe('euiCustomControl', () => { | |
| expect(result.current).toMatchInlineSnapshot(` | ||
| " | ||
| padding: 7px; | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it comes from different whitespace serializing in the latest |
||
| border: 1px solid #f5f7fc; | ||
| background: #FFF no-repeat center; | ||
|
|
||
|
|
@@ -200,7 +200,7 @@ describe('euiCustomControl', () => { | |
| expect(result.current).toMatchInlineSnapshot(` | ||
| " | ||
| padding: 15px; | ||
|
|
||
| border: 1px solid #f5f7fc; | ||
| background: #FFF no-repeat center; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,3 +15,4 @@ export { | |
| } from './react_warnings'; | ||
| export { sleep } from './sleep'; | ||
| export * from './emotion-prefix'; | ||
| export * from './react_version'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This definitely shouldn't be publicly available and won't even work when used outside test environment. I honestly thought none of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect, thanks Tomasz! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| export type ReactVersion = '16' | '17' | '18'; | ||
|
|
||
| /** | ||
| * Get major version of React that's currently used. | ||
| * | ||
| */ | ||
| export const getReactVersion = (): ReactVersion => { | ||
| const reactVersion = process.env.REACT_VERSION; | ||
| if (reactVersion !== undefined && ['16', '17', '18'].includes(reactVersion)) { | ||
| return reactVersion as ReactVersion; | ||
| } | ||
|
|
||
| return '18'; | ||
| }; | ||
|
|
||
| /** | ||
| * Invoke passed function when running on specified version(s) of React | ||
| */ | ||
| export const invokeOnReactVersion = ( | ||
| versionOrVersions: ReactVersion | ReactVersion[], | ||
| func: Function | ||
| ) => { | ||
| if (!Array.isArray(versionOrVersions)) { | ||
| versionOrVersions = [versionOrVersions]; | ||
| } | ||
|
|
||
| if (versionOrVersions.includes(getReactVersion())) { | ||
| func(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could replace this with the utility function but Jest 24 doesn't support TypeScript config files