[react-testing] do not use Promise.race to release stale act promises #2441

melnikov-s · 2022-10-18T15:05:15Z

Description

Fixes an issue where using Promise.race in order to unblock unresolved act promises breaks functionality that rely on very specific timings as Pormise.race adds an extra tick before the act can be resolved.

This was an issue with apollo-client and the error state of useLazyQuery. After some investigation it was determined that internally apollo-client will set the error result twice on the internal state. With the second time being ignored due to a shallow check. The timing of when the queue gets cleared after an act changes that behaviour and causes apollo-client to overwrite the first result.

As a result this PR uses a different approach that does not add an extra tick and keeps the same timings as before which resolves the apollo-client useLazyQuery issue and other "fragile" logic that depends on such timings.

I have created a snapshot and ran the CI on web with all tests passing as expected along with the changes that @BPScott did for the react-testing@v5 upgrade: https://github.com/Shopify/web/pull/75706

Once we merge this we can upgrade web to use react-testing@v5

melnikov-s · 2022-10-19T15:28:20Z

packages/react-testing/src/root.tsx

+         * into a set. If the result ever resolves we will remove the callback from the Set.
+         * If it doesn't we will call all callbacks in the Set when this root is destroyed which
+         * will unblock any stuck queues allowing subsequent tests to run
+         * Note: This can be cleanly achieved with a `Promise.race` but that adds an extra micro-task


Added a comment about Promise.race in case anyone would be tempted to refactor this in the future 😓

A test case that demonstrates this picky behavior that fails if you try to refactor this to use Promise.race would be a fantastic follow-up

Ah good call, I'll see if I can put a test case together later. It's going to quite tricky to extract a minimal test case from that apollo-client mess

I've been also been dealing with what can broadly be described as "apollo-client mess where task/microtask ordering is picky" too, I am full of sympathy.

ryanwilsonperkin · 2022-10-19T16:20:03Z

packages/react-testing/src/root.tsx

-    // flush the micro task to wait until react commits all pending updates.
-    await this.actPromise;
+    await mountedPromise;
+    this.actCallbacks.forEach((callback) => callback());


Won't these need to be awaited as well? And shouldn't they be invoked prior to unmounting?

Won't these need to be awaited as well?

These callbacks will popActScope and recursivelyFlushAsyncActWork if needed. There's nothing to await there.

And shouldn't they be invoked prior to unmounting?

Once the root is destroyed there should be no more assertions on it, we're calling these to flush the queue out and it's not really related to this instance of the root itself. I tried moving them up but it fails the tests as it appears that the final act in the unmount needs to be called and awaited on first.

I initially thought "should this be prior to the unmount" too. I was thinking there'd be an uptick in "you tried to act on a component that is unmounted" console warnings but that doesn't seem to be the case, thought admittedly the sheer noisyness of web's tests makes that hard to fully confirm.

BPScott

Fantastic sleuthing! And a huge thanks for doing the testing in web!

A test case that fails if the code tries to be refactored to use Promise.race would a great deterrent to stop people from speculative refactoring. That doesn't need block this PR though.

What's the relationship between this and #2438? I guess this PR superseeds that one aside from unskipping the test?

Merge when you're ready.

BPScott · 2022-10-19T17:03:00Z

packages/react-testing/src/root.tsx

+         * into a set. If the result ever resolves we will remove the callback from the Set.
+         * If it doesn't we will call all callbacks in the Set when this root is destroyed which
+         * will unblock any stuck queues allowing subsequent tests to run
+         * Note: This can be cleanly achieved with a `Promise.race` but that adds an extra micro-task


A test case that demonstrates this picky behavior that fails if you try to refactor this to use Promise.race would be a fantastic follow-up

melnikov-s · 2022-10-19T19:16:22Z

What's the relationship between this and #2438? I guess this PR superseeds that one aside from unskipping the test?

I think I'm still going to need

 if (isLegacyReact) {
      // flush macro task for react 17 only
      await new Promise((resolve) => enqueueTask(resolve));
    }

To get React 17 working

melnikov-s force-pushed the react-release-act-no-promise-race branch 6 times, most recently from e15c10e to d5348fc Compare October 19, 2022 13:58

[react-testing] do not use Promise.race to release stale act promises

e98fbe2

melnikov-s force-pushed the react-release-act-no-promise-race branch from d5348fc to e98fbe2 Compare October 19, 2022 14:22

Shopify deleted a comment from github-actions bot Oct 19, 2022

melnikov-s marked this pull request as ready for review October 19, 2022 15:24

melnikov-s requested a review from a team as a code owner October 19, 2022 15:24

melnikov-s requested review from frangelli, BPScott and ryanwilsonperkin October 19, 2022 15:24

melnikov-s commented Oct 19, 2022

View reviewed changes

ryanwilsonperkin reviewed Oct 19, 2022

View reviewed changes

ryanwilsonperkin approved these changes Oct 19, 2022

View reviewed changes

BPScott approved these changes Oct 19, 2022

View reviewed changes

melnikov-s merged commit cf70a15 into main Oct 19, 2022

melnikov-s deleted the react-release-act-no-promise-race branch October 19, 2022 19:16

shopify-github-actions-access bot mentioned this pull request Oct 19, 2022

Version Packages #2444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react-testing] do not use Promise.race to release stale act promises #2441

[react-testing] do not use Promise.race to release stale act promises #2441

melnikov-s commented Oct 18, 2022 •

edited

Loading

melnikov-s Oct 19, 2022

BPScott Oct 19, 2022

melnikov-s Oct 19, 2022

BPScott Oct 19, 2022

ryanwilsonperkin Oct 19, 2022

melnikov-s Oct 19, 2022 •

edited

Loading

BPScott Oct 19, 2022 •

edited

Loading

BPScott left a comment •

edited

Loading

BPScott Oct 19, 2022

melnikov-s commented Oct 19, 2022

[react-testing] do not use Promise.race to release stale act promises #2441

[react-testing] do not use Promise.race to release stale act promises #2441

Conversation

melnikov-s commented Oct 18, 2022 • edited Loading

Description

melnikov-s Oct 19, 2022

Choose a reason for hiding this comment

BPScott Oct 19, 2022

Choose a reason for hiding this comment

melnikov-s Oct 19, 2022

Choose a reason for hiding this comment

BPScott Oct 19, 2022

Choose a reason for hiding this comment

ryanwilsonperkin Oct 19, 2022

Choose a reason for hiding this comment

melnikov-s Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

BPScott Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

BPScott left a comment • edited Loading

Choose a reason for hiding this comment

BPScott Oct 19, 2022

Choose a reason for hiding this comment

melnikov-s commented Oct 19, 2022

melnikov-s commented Oct 18, 2022 •

edited

Loading

melnikov-s Oct 19, 2022 •

edited

Loading

BPScott Oct 19, 2022 •

edited

Loading

BPScott left a comment •

edited

Loading