React 18: Wrap non-RTL actions in act() #1401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 948 kB ℹ️ View Unchanged
|
jeremywiebe
force-pushed
the
jer/react-18-acting-up
branch
from
bab5608
to
7d5470b
Compare
This was referenced Jul 11, 2024
jeremywiebe
added a commit
that referenced
this pull request
Jul 11, 2024
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. This PR --> #1400 2. #1401 3. #1402 4. #1403 I thought I had the React 18 (#1064) upgrade all ready (and it was even approved), but when I returned to that PR, I noticed linter and test failures. This lead me to discover that I need to upgrade testing-library. This then breaks a bunch of tests (which I'll fix in future PRs). Issue: LEMS-1802 ## Test plan: This PR will introduce many test failures. That's ok, they'll be fixed in follow-up PRs so that hopefully each step is relatively easy to review. Author: jeremywiebe Reviewers: jeresig, #perseus, jandrade, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ⏭️ Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, ❌ Jest Coverage (ubuntu-latest, 20.x), ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1400
jeremywiebe
changed the base branch from
jer/react-18-upgrade-rtl
to
jer/react-18
An error occurred while trying to automatically change base from
jer/react-18-upgrade-rtl
to
jer/react-18
July 11, 2024 16:09
jeremywiebe
force-pushed
the
jer/react-18-acting-up
branch
from
487c943
to
5eae095
Compare
jeremywiebe
commented
Jul 12, 2024
| @@ -57,7 +57,7 @@ describe("<TabbarItem />", () => { | |||
| onClick={() => {}} | |||
| />, | |||
| ); | |||
| jest.runAllTimers(); | |||
| act(() => jest.runOnlyPendingTimers()); | |||
There was a problem hiding this comment.
React 18/RTL are much more sensitive to element updates. Most of the time when we call runOnlyPendingTimers() in Perseus tests it's because we're waiting for scheduled async work to complete and then re-render (via setTimeout, setState, etc).
jeremywiebe
commented
Jul 12, 2024
| @@ -0,0 +1,2 @@ | |||
| // eslint-disable-next-line import/no-unassigned-import | |||
| import "jest-extended"; // we love the side effects | |||
There was a problem hiding this comment.
This fixes a Typescript check issue where some of the Jest matchers we use (such as toBeTrue()) were no longer recognized by Typescript.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
jeresig
approved these changes
Jul 12, 2024
jeremywiebe
added a commit
that referenced
this pull request
Jul 15, 2024
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. #1400 2. #1401 3. This PR --> #1402 4. #1403 This PR fixes a few instances where there exists an RTL API for tests instead of making direct DOM object calls. RTL recommends this approach and this fixes a few more tests. Issue: LEMS-1802 ## Test plan: `yarn test` more tests pass ### Before ```sh $ yarn test ... Test Suites: 4 failed, 1 skipped, 184 passed, 188 of 189 total Tests: 9 failed, 14 skipped, 2564 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 24.056 s, estimated 25 s Ran all test suites. error Command failed with exit code 1. ``` ### After ```sh $ yarn test ... Test Suites: 3 failed, 1 skipped, 185 passed, 188 of 189 total Tests: 6 failed, 14 skipped, 2567 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 20.517 s, estimated 24 s Ran all test suites. error Command failed with exit code 1. ``` Close! :) Author: jeremywiebe Reviewers: jeremywiebe, jeresig, #perseus, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ⏭️ Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1402
jeremywiebe
added a commit
that referenced
this pull request
Jul 15, 2024
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. #1400 2. #1401 3. #1402 4. This PR --> #1403 A few tests needed larger changes than simply wrapping the actions in `act()` (see #1402). This PR covers the remaining tests that were failing for various reasons. In some cases the tests were largely re-written (sometimes for better test clarity while re-working them to pass). In the case of the Zoomable tests, we were waiting for the text to be updated in the element, but the styling changes had not yet settled as the updates caused multiple `setState()` calls to be made which all needed to be called and then resolved by a subsequent render (thus the new `waitForStyle()` helper). In the Graded Group test update, I'm not sure how it worked as written. In the test that was fixed, we were testing mobile and in that case the "Check" button reads "Try again" on the second and subsequent attempts. In the Group tests, I needed to wrap some actions in `act()` as usual, but I also took the opportunity to change the element finding logic away from the `getAll...()` functions to depend more on the aria labels. This makes these tests read more clearly (instead of accessing item at index `[0]` we get the textbox with the string `"value rounded to the nearest ten"`). Lastly, there was one outdated snapshot due to a test file being renamed. Issue: LEMS-1802 ## Test plan: `yarn test` all tests now pass! ### Before ```sh $ yarn test ... Test Suites: 3 failed, 1 skipped, 185 passed, 188 of 189 total Tests: 6 failed, 14 skipped, 2567 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 22.175 s, estimated 27 s Ran all test suites. ``` ### After ```sh $ yarn test ... Test Suites: 1 skipped, 188 passed, 188 of 189 total Tests: 14 skipped, 2573 passed, 2587 total Snapshots: 159 passed, 159 total Time: 26.765 s Ran all test suites. ``` Author: jeremywiebe Reviewers: jeremywiebe, jeresig, #perseus, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ✅ codecov/patch, ✅ codecov/project, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ✅ gerald, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1403
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR is the part of a chain of PRs upgrading Perseus to React 18.
It appears that React 18 and RTL are more sensitive to not having mutating calls wrapped in
act(). The changes in this PR are all simple changes that wrap function calls that cause DOM changes inact()calls. There are a few special cases that are failing yet, but this PR cleans up the straightforward ones.Issue: LEMS-1802
Test plan:
yarn testshould have many fewer test failures than PR #1400Upcoming PRs will address the remaining test failures.
Before
After