[react-testing] fixed react queue emptying for React 17 #2438
Conversation
melnikov-s
force-pushed
the
react-testing-act-for-react-17
branch
from
6a7c8cc to
ce0c718
Compare
| @@ -179,9 +179,6 @@ describe('@shopify/react-testing', () => { | |||
| wrapper.act(() => new Promise(() => {})); | |||
| await wrapper.destroy(); | |||
|
|
|||
| // React 17 will fail without this await | |||
| await new Promise((resolve) => setTimeout(resolve, 0)); | |||
There was a problem hiding this comment.
This is no longer needed, my guess is to why the tests were flaky was due to mocked timers that were not being restored. I found one such place here:
| // https://github.com/facebook/react/blob/12adaffef7105e2714f82651ea51936c563fe15c/packages/shared/enqueueTask.js#L13 | ||
| let enqueueTaskImpl: any = null; | ||
|
|
||
| export function enqueueTask(task: (v: any) => void) { |
There was a problem hiding this comment.
This is how the queue gets emptied out in older version of react. It creates a macro task either with setImmediate or postMessage . We're probably ok doing only the setImmediate here but decided to copy the entire code to be sure and match what react is doing.
packages/react-testing/src/root.tsx
Outdated
| result, | ||
| this.actPromise, | ||
| ]) as unknown as Promise<void>; | ||
|
|
||
| this.actPromises.push(promise); |
There was a problem hiding this comment.
We collect all the promises we need to wait on as Promise.race([Promise.resolve()] does not resolve at the same time as Promise.resolve() (it adds a tick). So best to wait on the same promise that gets returned to act
melnikov-s
force-pushed
the
react-testing-act-for-react-17
branch
from
ce0c718 to
f898f1e
Compare
There was a problem hiding this comment.
Heck yeah! Thank you so much for digging into this. I see no glaring issue with the code (not that I saw any problem with the prior code either).
Ryan ended up merging #2435 not spotting this PR was super close. Can you want to revert that commit so we can see the e2e test passes in react 17.
Saw that, this PR is rebased on top of those changes and is running the e2e tests on React 17. I've been restarting the runs periodically and so far no e2e tests failures here 🤞 . I'll snapshot it and ran against web to see what the results are now. I'm still expecting some failures due to |
melnikov-s
force-pushed
the
react-testing-act-for-react-17
branch
from
f898f1e to
27067e1
Compare
melnikov-s
force-pushed
the
react-testing-act-for-react-17
branch
from
27067e1 to
b9eadd5
Compare
|
Did another snapshot run on web just in case and it continues to pass (https://buildkite.com/shopify/web-ci-builder/builds/555602). React 17 e2e tests have been stable. Merging. |
Description
#2352 Introduced automatic act queue emptying when a destroying a root wrapper. It fixed this fully in React 18 but not for older versions of react. This resulted in flaky tests for quilt as well as potentially surprising behaviour for React 17 users upgrading to react-testing@5.
This PR also adds queue emptying for older version of React.