Skip to content

Conversation

@tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Jul 13, 2023

Summary

This PR adds act() wrappers in a few places to ensure correct execution when running on React 18 and earlier versions.

QA

  1. Checkout this branch locally
  2. Run yarn to install dependencies
  3. Run REACT_VERSION=17 yarn test-unit and ensure there's Running tests on React v17 printed out and there are no failing tests
  4. Run yarn test-unit and confirm Running tests on React v18 is printed out and there are some failing tests, but none are caused by act() warnings.

@tkajtoch tkajtoch requested a review from a team July 13, 2023 10:09
@tkajtoch tkajtoch self-assigned this Jul 13, 2023
@tkajtoch tkajtoch force-pushed the test/react-18-act-adjustments branch from c72d471 to fcc0e7e Compare July 13, 2023 15:44
"@svgr/core": "8.0.0",
"@svgr/plugin-jsx": "^8.0.1",
"@svgr/plugin-svgo": "^8.0.1",
"@testing-library/dom": "^8.12.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@testing-library/dom is unused at this point as this PR switches to using fireEvent imported from @testing-library/react (and automatically wrapped in act() so we don't need to do that manually)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - was that a recent change in @testing-library/react? Or did I just add a library I didn't need to when I was originally setting up RTL? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it existed for quite some time now, but it was never well documented and people get confused all the time!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! Thanks a million for the cleanup in that case! 💯


component.setState({ isListOpen: true });
act(() => {
component.setState({ isListOpen: true });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React 17 act() can only return void or Promise so all one-liners like this one need to be inserted in a block

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM!

@tkajtoch tkajtoch force-pushed the test/react-18-act-adjustments branch from fcc0e7e to d119980 Compare July 13, 2023 21:24
@tkajtoch
Copy link
Member Author

FYI I switched to importing act from @testing-library/react as RTL docs recommend this. It's a simple wrapper over react-dom/test-utils that also works just fine with enzyme tests using act.

@cee-chen
Copy link
Contributor

FYI I switched to importing act from @testing-library/react as RTL docs recommend this. It's a simple wrapper over react-dom/test-utils that also works just fine with enzyme tests using act.

Nice, that totally makes sense!

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a little longer to yarn about code abstraction and answer my various questions Tomasz! Changes look great!

@tkajtoch tkajtoch merged commit 33861c2 into elastic:feature/react-18 Jul 14, 2023
tkajtoch added a commit that referenced this pull request Jul 24, 2023
* test: replace @testing-library/dom with @testing-library/react as the latter exports are wrapped in act()

* test: wrap function calls with act() that need it

* test: use act() wrapper for jest.advanceTimersByTime calls and add actAdvanceTimersByTime utility function

* test: wait for element to be rendered before taking a snapshot to fix act() warning
tkajtoch added a commit that referenced this pull request Jul 26, 2023
* test: replace @testing-library/dom with @testing-library/react as the latter exports are wrapped in act()

* test: wrap function calls with act() that need it

* test: use act() wrapper for jest.advanceTimersByTime calls and add actAdvanceTimersByTime utility function

* test: wait for element to be rendered before taking a snapshot to fix act() warning
tkajtoch added a commit that referenced this pull request Jul 28, 2023
* test: replace @testing-library/dom with @testing-library/react as the latter exports are wrapped in act()

* test: wrap function calls with act() that need it

* test: use act() wrapper for jest.advanceTimersByTime calls and add actAdvanceTimersByTime utility function

* test: wait for element to be rendered before taking a snapshot to fix act() warning
tkajtoch added a commit that referenced this pull request Jul 31, 2023
* test: replace @testing-library/dom with @testing-library/react as the latter exports are wrapped in act()

* test: wrap function calls with act() that need it

* test: use act() wrapper for jest.advanceTimersByTime calls and add actAdvanceTimersByTime utility function

* test: wait for element to be rendered before taking a snapshot to fix act() warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants