[Concurrent React@18] Switch testing library to createRoot #213367
[Concurrent React@18] Switch testing library to createRoot #213367Dosant merged 43 commits intoelastic:mainfrom
createRoot #213367Conversation
976e424 to
85852b2
Compare
createRoot
createRoot createRoot
createRoot createRoot
jkelas
left a comment
There was a problem hiding this comment.
Rule management changes look good:
pr-files-by-owner 213367
### elastic/security-detection-rule-management
* x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/logic/use_dissasociate_exception_list.test.ts
* x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/filter_panel.test.tsx
* x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/rule_management/__integration_tests__/rules_upgrade/test_utils/rule_upgrade_flyout.tsx
Tests PASS:
PASS x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/logic/use_dissasociate_exception_list.test.ts (5.833 s)
useDisassociateExceptionList
✓ initializes hook (8 ms)
PASS x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/filter_panel.test.tsx (44.739 s)
CoverageOverviewFiltersPanel
✓ it correctly populates rule activity filter state (788 ms)
✓ it correctly populates rule source filter state (266 ms)
✓ it correctly populates search filter state (91 ms)
PASS x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/rule_management/__integration_tests__/rules_upgrade/diff_view_options.test.ts (121.43 s)
PASS x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/rule_management/__integration_tests__/rules_upgrade/rule_upgrade_button.test.ts (17.974 s)
I only left one minor nit comment, for consideration.
| const wrapper = renderFiltersPanel(); | ||
|
|
||
| wrapper.getByTestId('coverageOverviewRuleSourceFilterButton').click(); | ||
| await userEvent.click(wrapper.getByTestId('coverageOverviewRuleSourceFilterButton')); |
There was a problem hiding this comment.
Nit: It would be a bit nicer to use the setup function and then type 'user.click', like it is done in other places already and also suggested by the documentation of the testing library https://testing-library.com/docs/user-event/intro/#writing-tests-with-userevent.
There was a problem hiding this comment.
Using the method directly from userEvent without calling .setup() is also acceptable as long as it's from the default export, this is also mentioned in the link you shared, more on this here see https://testing-library.com/docs/user-event/setup/#direct-apis,
nickpeihl
left a comment
There was a problem hiding this comment.
lgtm! thanks for updating the test.
|
Backport: Since the 8.19 branch doesn't have React 18 (we're discussing whether it should), I will try to manually backport most of the backward-compatible test changes in this PR. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Page load bundle
History
|
…#213367) Closes elastic#218076. Part of the preparation for migrating Kibana to React’s createRoot ([Epic](elastic/kibana-team#1564)). ## What’s in this PR - Switch to `createRoot` in tests: Updates `@testing-library/react` to use `createRoot` by default. All unit tests using Testing Library now run in concurrent mode. See commit: elastic@8e51e07 - Test updates: Most test failures from the switch have been addressed. About a dozen tests still explicitly set `legacyRoot: true`. These will need closer review by the owning teams during app migrations. - Enzyme tests: Enzyme tests continue to use the React 17 adapter and run in legacy mode. [Current plan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0) is to migrate away from Enzyme before upgrading to React 19. ## Background When we upgraded to React 18, we also updated `@testing-library/react`, which by default uses `createRoot`. To avoid dealing with concurrent mode failures early, we temporarily forced Testing Library to use `legacyRoot` (`ReactDOM.render`). This PR removes that override and fixes the resulting test issues, completing the move to concurrent root for Testing Library tests. ### Common Failures #### 🔴 `el.click()` A common testing mistake is using el.click() and immediately checking for a DOM update: ``` el.click(); expect(el).toHaveAttribute('state-updated'); ``` This often fails with Concurrent React, because state updates might not be synchronous anymore. Directly calling `el.click()` doesn't automatically trigger React’s update cycle (`act`), so your test can read outdated DOM. Instead, you should either manually wrap the interaction in `act`, or (better) use `userEvent.click`, which already uses `act` internally and simulates real user behavior more accurately: ```diff - el.click(); + await userEvent.click(el); expect(el).toHaveAttribute('state-updated'); ``` #### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))` Another common mistake is wrapping the render call inside act: ``` await act(async () => { render(<MyComponent />); }); ``` This is sometimes done to "mute" warnings about Promises resolving inside `useEffect`. However, wrapping `render` in `act` manually breaks a lot of tests in Concurrent React, because the library (like React Testing Library) already wraps render in act internally. Manually adding act here can cause unexpected behavior, like missing updates or wrong timing. The approach I took was to remove the manual `act` around `render` in places where tests started failing with Concurrent React, even if, in some cases, it means seeing `act` warnings in the console. This is safer for correctness and allows the tests to pass reliably. To properly mute such warnings, the right way would be to wrap the actual resolved Promises (like those inside useEffect) in act.However, since doing that depends a lot on the specific test setup, and could vary case-by-case, I chose not to try to fix it myself. Teams are welcome to follow up if they wish. ### 🟡 In specific tests we keep `legacyMode: true` When it wasn't immediately clear to me what caused the failure or when the tests were checking React internals, like the number of re-renders, I decided to keep that test running in legacy mode by using the option `legacyRoot: true` in `render`. The idea behind these in-place overrides is that when we're ready to start migrating the runtime to concurrent mode, the owning teams will need to take a closer look at those tests when moving their apps to the concurrent root. (cherry picked from commit 24d4c8a) # Conflicts: # src/platform/packages/shared/kbn-test/src/jest/setup/react_testing_library.js # src/platform/packages/shared/kbn-unified-tabs/src/components/tab/tab.test.tsx # src/platform/packages/shared/shared-ux/error_boundary/src/ui/error_boundary.test.tsx # src/platform/packages/shared/shared-ux/error_boundary/src/ui/section_error_boundary.test.tsx # src/platform/plugins/shared/home/public/application/components/tutorial/tutorial.test.tsx # src/platform/plugins/shared/unified_doc_viewer/public/components/doc_viewer_table/table_cell_value.test.tsx # x-pack/platform/packages/shared/kbn-elastic-assistant/impl/assistant/api/chat_complete/use_chat_complete.test.ts # x-pack/platform/packages/shared/kbn-elastic-assistant/impl/assistant/settings/settings_context_menu/settings_context_menu.test.tsx # x-pack/platform/plugins/shared/automatic_import/public/components/create_integration/create_automatic_import/flyout/cel_configuration/steps/upload_spec_step/api_definition_input.test.tsx # x-pack/platform/plugins/shared/automatic_import/public/components/create_integration/create_automatic_import/flyout/cel_configuration/steps/upload_spec_step/upload_spec_step.test.tsx # x-pack/platform/plugins/shared/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx # x-pack/platform/plugins/shared/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel_wrapper.test.tsx # x-pack/platform/plugins/shared/security/public/account_management/user_profile/user_profile.test.tsx # x-pack/platform/plugins/shared/spaces/public/management/edit_space/edit_space_general_tab.test.tsx # x-pack/solutions/observability/plugins/apm/public/components/app/transaction_details/waterfall_with_summary/waterfall_container/waterfall_container.test.tsx # x-pack/solutions/security/plugins/security_solution/public/common/test/eui/super_select.ts # x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/pages/rule_management/__integration_tests__/rules_upgrade/test_utils/rule_upgrade_flyout.tsx # x-pack/solutions/security/plugins/security_solution/public/detections/components/alert_summary/search_bar/integrations_filter_button.test.tsx # x-pack/solutions/security/plugins/security_solution/public/detections/components/alert_summary/table/more_actions_row_control_column.test.tsx # x-pack/solutions/security/plugins/security_solution/public/detections/pages/alert_summary/alert_summary.test.tsx # x-pack/solutions/security/plugins/security_solution/public/flyout/ai_for_soc/components/settings_menu.test.tsx # x-pack/solutions/security/plugins/security_solution/public/flyout/ai_for_soc/components/take_action_button.test.tsx
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…createRoot` (#213367) (#220500) # Backport This will backport the following commits from `main` to `8.19`: - [[Concurrent React@18] Switch testing library to `createRoot` (#213367)](#213367), but without actually enabling concurrent root for tests. Just the backward compatible unit test fixes <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Anton Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2025-05-08T11:59:44Z","message":"[Concurrent React@18] Switch testing library to `createRoot` (#213367)\n\nCloses #218076. \nPart of the preparation for migrating Kibana to React’s createRoot\n([Epic](https://github.com/elastic/kibana-team/issues/1564)).\n\n## What’s in this PR\n\n- Switch to `createRoot` in tests: Updates `@testing-library/react` to\nuse `createRoot` by default. All unit tests using Testing Library now\nrun in concurrent mode. See commit:\nhttps://github.com//pull/213367/commits/8e51e07054ab5d0993e9856a851c55716e90190b\n- Test updates: Most test failures from the switch have been addressed.\nAbout a dozen tests still explicitly set `legacyRoot: true`. These will\nneed closer review by the owning teams during app migrations.\n- Enzyme tests: Enzyme tests continue to use the React 17 adapter and\nrun in legacy mode. [Current\nplan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0)\nis to migrate away from Enzyme before upgrading to React 19.\n\n## Background\n\nWhen we upgraded to React 18, we also updated `@testing-library/react`,\nwhich by default uses `createRoot`.\nTo avoid dealing with concurrent mode failures early, we temporarily\nforced Testing Library to use `legacyRoot` (`ReactDOM.render`).\n\nThis PR removes that override and fixes the resulting test issues,\ncompleting the move to concurrent root for Testing Library tests.\n\n\n### Common Failures\n\n#### 🔴 `el.click()` \n\nA common testing mistake is using el.click() and immediately checking\nfor a DOM update:\n```\nel.click();\nexpect(el).toHaveAttribute('state-updated');\n```\n\nThis often fails with Concurrent React, because state updates might not\nbe synchronous anymore.\nDirectly calling `el.click()` doesn't automatically trigger React’s\nupdate cycle (`act`), so your test can read outdated DOM.\n\nInstead, you should either manually wrap the interaction in `act`, or\n(better) use `userEvent.click`, which already uses `act` internally and\nsimulates real user behavior more accurately:\n\n```diff\n- el.click();\n+ await userEvent.click(el);\nexpect(el).toHaveAttribute('state-updated');\n```\n\n\n#### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))`\n\nAnother common mistake is wrapping the render call inside act:\n\n```\nawait act(async () => {\n render(<MyComponent />);\n});\n```\n\nThis is sometimes done to \"mute\" warnings about Promises resolving\ninside `useEffect`.\nHowever, wrapping `render` in `act` manually breaks a lot of tests in\nConcurrent React, because the library (like React Testing Library)\nalready wraps render in act internally. Manually adding act here can\ncause unexpected behavior, like missing updates or wrong timing.\n\nThe approach I took was to remove the manual `act` around `render` in\nplaces where tests started failing with Concurrent React, even if, in\nsome cases, it means seeing `act` warnings in the console. This is safer\nfor correctness and allows the tests to pass reliably.\n\nTo properly mute such warnings, the right way would be to wrap the\nactual resolved Promises (like those inside useEffect) in act.However,\nsince doing that depends a lot on the specific test setup, and could\nvary case-by-case, I chose not to try to fix it myself. Teams are\nwelcome to follow up if they wish.\n\n### 🟡 In specific tests we keep `legacyMode: true`\n\nWhen it wasn't immediately clear to me what caused the failure or when\nthe tests were checking React internals, like the number of re-renders,\nI decided to keep that test running in legacy mode by using the option\n`legacyRoot: true` in `render`.\n\nThe idea behind these in-place overrides is that when we're ready to\nstart migrating the runtime to concurrent mode, the owning teams will\nneed to take a closer look at those tests when moving their apps to the\nconcurrent root.","sha":"24d4c8aa0b60156e87a614bae5e7d6489c8c5972","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","Team:Fleet","Team:SharedUX","ci:project-deploy-observability","Team:obs-ux-infra_services","Team:obs-ux-management","React@18","v9.1.0","v8.19.0"],"title":"[Concurrent React@18] Switch testing library to `createRoot` ","number":213367,"url":"https://github.com/elastic/kibana/pull/213367","mergeCommit":{"message":"[Concurrent React@18] Switch testing library to `createRoot` (#213367)\n\nCloses #218076. \nPart of the preparation for migrating Kibana to React’s createRoot\n([Epic](https://github.com/elastic/kibana-team/issues/1564)).\n\n## What’s in this PR\n\n- Switch to `createRoot` in tests: Updates `@testing-library/react` to\nuse `createRoot` by default. All unit tests using Testing Library now\nrun in concurrent mode. See commit:\nhttps://github.com//pull/213367/commits/8e51e07054ab5d0993e9856a851c55716e90190b\n- Test updates: Most test failures from the switch have been addressed.\nAbout a dozen tests still explicitly set `legacyRoot: true`. These will\nneed closer review by the owning teams during app migrations.\n- Enzyme tests: Enzyme tests continue to use the React 17 adapter and\nrun in legacy mode. [Current\nplan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0)\nis to migrate away from Enzyme before upgrading to React 19.\n\n## Background\n\nWhen we upgraded to React 18, we also updated `@testing-library/react`,\nwhich by default uses `createRoot`.\nTo avoid dealing with concurrent mode failures early, we temporarily\nforced Testing Library to use `legacyRoot` (`ReactDOM.render`).\n\nThis PR removes that override and fixes the resulting test issues,\ncompleting the move to concurrent root for Testing Library tests.\n\n\n### Common Failures\n\n#### 🔴 `el.click()` \n\nA common testing mistake is using el.click() and immediately checking\nfor a DOM update:\n```\nel.click();\nexpect(el).toHaveAttribute('state-updated');\n```\n\nThis often fails with Concurrent React, because state updates might not\nbe synchronous anymore.\nDirectly calling `el.click()` doesn't automatically trigger React’s\nupdate cycle (`act`), so your test can read outdated DOM.\n\nInstead, you should either manually wrap the interaction in `act`, or\n(better) use `userEvent.click`, which already uses `act` internally and\nsimulates real user behavior more accurately:\n\n```diff\n- el.click();\n+ await userEvent.click(el);\nexpect(el).toHaveAttribute('state-updated');\n```\n\n\n#### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))`\n\nAnother common mistake is wrapping the render call inside act:\n\n```\nawait act(async () => {\n render(<MyComponent />);\n});\n```\n\nThis is sometimes done to \"mute\" warnings about Promises resolving\ninside `useEffect`.\nHowever, wrapping `render` in `act` manually breaks a lot of tests in\nConcurrent React, because the library (like React Testing Library)\nalready wraps render in act internally. Manually adding act here can\ncause unexpected behavior, like missing updates or wrong timing.\n\nThe approach I took was to remove the manual `act` around `render` in\nplaces where tests started failing with Concurrent React, even if, in\nsome cases, it means seeing `act` warnings in the console. This is safer\nfor correctness and allows the tests to pass reliably.\n\nTo properly mute such warnings, the right way would be to wrap the\nactual resolved Promises (like those inside useEffect) in act.However,\nsince doing that depends a lot on the specific test setup, and could\nvary case-by-case, I chose not to try to fix it myself. Teams are\nwelcome to follow up if they wish.\n\n### 🟡 In specific tests we keep `legacyMode: true`\n\nWhen it wasn't immediately clear to me what caused the failure or when\nthe tests were checking React internals, like the number of re-renders,\nI decided to keep that test running in legacy mode by using the option\n`legacyRoot: true` in `render`.\n\nThe idea behind these in-place overrides is that when we're ready to\nstart migrating the runtime to concurrent mode, the owning teams will\nneed to take a closer look at those tests when moving their apps to the\nconcurrent root.","sha":"24d4c8aa0b60156e87a614bae5e7d6489c8c5972"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/213367","number":213367,"mergeCommit":{"message":"[Concurrent React@18] Switch testing library to `createRoot` (#213367)\n\nCloses #218076. \nPart of the preparation for migrating Kibana to React’s createRoot\n([Epic](https://github.com/elastic/kibana-team/issues/1564)).\n\n## What’s in this PR\n\n- Switch to `createRoot` in tests: Updates `@testing-library/react` to\nuse `createRoot` by default. All unit tests using Testing Library now\nrun in concurrent mode. See commit:\nhttps://github.com//pull/213367/commits/8e51e07054ab5d0993e9856a851c55716e90190b\n- Test updates: Most test failures from the switch have been addressed.\nAbout a dozen tests still explicitly set `legacyRoot: true`. These will\nneed closer review by the owning teams during app migrations.\n- Enzyme tests: Enzyme tests continue to use the React 17 adapter and\nrun in legacy mode. [Current\nplan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0)\nis to migrate away from Enzyme before upgrading to React 19.\n\n## Background\n\nWhen we upgraded to React 18, we also updated `@testing-library/react`,\nwhich by default uses `createRoot`.\nTo avoid dealing with concurrent mode failures early, we temporarily\nforced Testing Library to use `legacyRoot` (`ReactDOM.render`).\n\nThis PR removes that override and fixes the resulting test issues,\ncompleting the move to concurrent root for Testing Library tests.\n\n\n### Common Failures\n\n#### 🔴 `el.click()` \n\nA common testing mistake is using el.click() and immediately checking\nfor a DOM update:\n```\nel.click();\nexpect(el).toHaveAttribute('state-updated');\n```\n\nThis often fails with Concurrent React, because state updates might not\nbe synchronous anymore.\nDirectly calling `el.click()` doesn't automatically trigger React’s\nupdate cycle (`act`), so your test can read outdated DOM.\n\nInstead, you should either manually wrap the interaction in `act`, or\n(better) use `userEvent.click`, which already uses `act` internally and\nsimulates real user behavior more accurately:\n\n```diff\n- el.click();\n+ await userEvent.click(el);\nexpect(el).toHaveAttribute('state-updated');\n```\n\n\n#### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))`\n\nAnother common mistake is wrapping the render call inside act:\n\n```\nawait act(async () => {\n render(<MyComponent />);\n});\n```\n\nThis is sometimes done to \"mute\" warnings about Promises resolving\ninside `useEffect`.\nHowever, wrapping `render` in `act` manually breaks a lot of tests in\nConcurrent React, because the library (like React Testing Library)\nalready wraps render in act internally. Manually adding act here can\ncause unexpected behavior, like missing updates or wrong timing.\n\nThe approach I took was to remove the manual `act` around `render` in\nplaces where tests started failing with Concurrent React, even if, in\nsome cases, it means seeing `act` warnings in the console. This is safer\nfor correctness and allows the tests to pass reliably.\n\nTo properly mute such warnings, the right way would be to wrap the\nactual resolved Promises (like those inside useEffect) in act.However,\nsince doing that depends a lot on the specific test setup, and could\nvary case-by-case, I chose not to try to fix it myself. Teams are\nwelcome to follow up if they wish.\n\n### 🟡 In specific tests we keep `legacyMode: true`\n\nWhen it wasn't immediately clear to me what caused the failure or when\nthe tests were checking React internals, like the number of re-renders,\nI decided to keep that test running in legacy mode by using the option\n`legacyRoot: true` in `render`.\n\nThe idea behind these in-place overrides is that when we're ready to\nstart migrating the runtime to concurrent mode, the owning teams will\nneed to take a closer look at those tests when moving their apps to the\nconcurrent root.","sha":"24d4c8aa0b60156e87a614bae5e7d6489c8c5972"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
…#213367) Closes elastic#218076. Part of the preparation for migrating Kibana to React’s createRoot ([Epic](elastic/kibana-team#1564)). ## What’s in this PR - Switch to `createRoot` in tests: Updates `@testing-library/react` to use `createRoot` by default. All unit tests using Testing Library now run in concurrent mode. See commit: elastic@8e51e07 - Test updates: Most test failures from the switch have been addressed. About a dozen tests still explicitly set `legacyRoot: true`. These will need closer review by the owning teams during app migrations. - Enzyme tests: Enzyme tests continue to use the React 17 adapter and run in legacy mode. [Current plan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0) is to migrate away from Enzyme before upgrading to React 19. ## Background When we upgraded to React 18, we also updated `@testing-library/react`, which by default uses `createRoot`. To avoid dealing with concurrent mode failures early, we temporarily forced Testing Library to use `legacyRoot` (`ReactDOM.render`). This PR removes that override and fixes the resulting test issues, completing the move to concurrent root for Testing Library tests. ### Common Failures #### 🔴 `el.click()` A common testing mistake is using el.click() and immediately checking for a DOM update: ``` el.click(); expect(el).toHaveAttribute('state-updated'); ``` This often fails with Concurrent React, because state updates might not be synchronous anymore. Directly calling `el.click()` doesn't automatically trigger React’s update cycle (`act`), so your test can read outdated DOM. Instead, you should either manually wrap the interaction in `act`, or (better) use `userEvent.click`, which already uses `act` internally and simulates real user behavior more accurately: ```diff - el.click(); + await userEvent.click(el); expect(el).toHaveAttribute('state-updated'); ``` #### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))` Another common mistake is wrapping the render call inside act: ``` await act(async () => { render(<MyComponent />); }); ``` This is sometimes done to "mute" warnings about Promises resolving inside `useEffect`. However, wrapping `render` in `act` manually breaks a lot of tests in Concurrent React, because the library (like React Testing Library) already wraps render in act internally. Manually adding act here can cause unexpected behavior, like missing updates or wrong timing. The approach I took was to remove the manual `act` around `render` in places where tests started failing with Concurrent React, even if, in some cases, it means seeing `act` warnings in the console. This is safer for correctness and allows the tests to pass reliably. To properly mute such warnings, the right way would be to wrap the actual resolved Promises (like those inside useEffect) in act.However, since doing that depends a lot on the specific test setup, and could vary case-by-case, I chose not to try to fix it myself. Teams are welcome to follow up if they wish. ### 🟡 In specific tests we keep `legacyMode: true` When it wasn't immediately clear to me what caused the failure or when the tests were checking React internals, like the number of re-renders, I decided to keep that test running in legacy mode by using the option `legacyRoot: true` in `render`. The idea behind these in-place overrides is that when we're ready to start migrating the runtime to concurrent mode, the owning teams will need to take a closer look at those tests when moving their apps to the concurrent root.
…#213367) Closes elastic#218076. Part of the preparation for migrating Kibana to React’s createRoot ([Epic](elastic/kibana-team#1564)). ## What’s in this PR - Switch to `createRoot` in tests: Updates `@testing-library/react` to use `createRoot` by default. All unit tests using Testing Library now run in concurrent mode. See commit: elastic@8e51e07 - Test updates: Most test failures from the switch have been addressed. About a dozen tests still explicitly set `legacyRoot: true`. These will need closer review by the owning teams during app migrations. - Enzyme tests: Enzyme tests continue to use the React 17 adapter and run in legacy mode. [Current plan](https://docs.google.com/document/d/1CXybQiBAtXt3Kay0j_CJxWO7bZ2EYYhaveK4fban2-M/edit?tab=t.kfgvma8ti7q0) is to migrate away from Enzyme before upgrading to React 19. ## Background When we upgraded to React 18, we also updated `@testing-library/react`, which by default uses `createRoot`. To avoid dealing with concurrent mode failures early, we temporarily forced Testing Library to use `legacyRoot` (`ReactDOM.render`). This PR removes that override and fixes the resulting test issues, completing the move to concurrent root for Testing Library tests. ### Common Failures #### 🔴 `el.click()` A common testing mistake is using el.click() and immediately checking for a DOM update: ``` el.click(); expect(el).toHaveAttribute('state-updated'); ``` This often fails with Concurrent React, because state updates might not be synchronous anymore. Directly calling `el.click()` doesn't automatically trigger React’s update cycle (`act`), so your test can read outdated DOM. Instead, you should either manually wrap the interaction in `act`, or (better) use `userEvent.click`, which already uses `act` internally and simulates real user behavior more accurately: ```diff - el.click(); + await userEvent.click(el); expect(el).toHaveAttribute('state-updated'); ``` #### 🔴 Wrapping `render` call inside `act` `act(() => render(<App/>))` Another common mistake is wrapping the render call inside act: ``` await act(async () => { render(<MyComponent />); }); ``` This is sometimes done to "mute" warnings about Promises resolving inside `useEffect`. However, wrapping `render` in `act` manually breaks a lot of tests in Concurrent React, because the library (like React Testing Library) already wraps render in act internally. Manually adding act here can cause unexpected behavior, like missing updates or wrong timing. The approach I took was to remove the manual `act` around `render` in places where tests started failing with Concurrent React, even if, in some cases, it means seeing `act` warnings in the console. This is safer for correctness and allows the tests to pass reliably. To properly mute such warnings, the right way would be to wrap the actual resolved Promises (like those inside useEffect) in act.However, since doing that depends a lot on the specific test setup, and could vary case-by-case, I chose not to try to fix it myself. Teams are welcome to follow up if they wish. ### 🟡 In specific tests we keep `legacyMode: true` When it wasn't immediately clear to me what caused the failure or when the tests were checking React internals, like the number of re-renders, I decided to keep that test running in legacy mode by using the option `legacyRoot: true` in `render`. The idea behind these in-place overrides is that when we're ready to start migrating the runtime to concurrent mode, the owning teams will need to take a closer look at those tests when moving their apps to the concurrent root.
Closes #218076.
Part of the preparation for migrating Kibana to React’s createRoot (Epic).
What’s in this PR
createRootin tests: Updates@testing-library/reactto usecreateRootby default. All unit tests using Testing Library now run in concurrent mode. See commit: 8e51e07legacyRoot: true. These will need closer review by the owning teams during app migrations.Background
When we upgraded to React 18, we also updated
@testing-library/react, which by default usescreateRoot.To avoid dealing with concurrent mode failures early, we temporarily forced Testing Library to use
legacyRoot(ReactDOM.render).This PR removes that override and fixes the resulting test issues, completing the move to concurrent root for Testing Library tests.
Common Failures
🔴
el.click()A common testing mistake is using el.click() and immediately checking for a DOM update:
This often fails with Concurrent React, because state updates might not be synchronous anymore.
Directly calling
el.click()doesn't automatically trigger React’s update cycle (act), so your test can read outdated DOM.Instead, you should either manually wrap the interaction in
act, or (better) useuserEvent.click, which already usesactinternally and simulates real user behavior more accurately:🔴 Wrapping
rendercall insideactact(() => render(<App/>))Another common mistake is wrapping the render call inside act:
This is sometimes done to "mute" warnings about Promises resolving inside
useEffect.However, wrapping
renderinactmanually breaks a lot of tests in Concurrent React, because the library (like React Testing Library) already wraps render in act internally. Manually adding act here can cause unexpected behavior, like missing updates or wrong timing.The approach I took was to remove the manual
actaroundrenderin places where tests started failing with Concurrent React, even if, in some cases, it means seeingactwarnings in the console. This is safer for correctness and allows the tests to pass reliably.To properly mute such warnings, the right way would be to wrap the actual resolved Promises (like those inside useEffect) in act.However, since doing that depends a lot on the specific test setup, and could vary case-by-case, I chose not to try to fix it myself. Teams are welcome to follow up if they wish.
🟡 In specific tests we keep
legacyMode: trueWhen it wasn't immediately clear to me what caused the failure or when the tests were checking React internals, like the number of re-renders, I decided to keep that test running in legacy mode by using the option
legacyRoot: trueinrender.The idea behind these in-place overrides is that when we're ready to start migrating the runtime to concurrent mode, the owning teams will need to take a closer look at those tests when moving their apps to the concurrent root.