Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Prevent "missing act" warning for queued microtasks #1137

Merged
merged 5 commits into from
Feb 16, 2023

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 8, 2022

What:

Closes #1125
Closes #1051

Why:

Once the callback passed to waitFor resolves, there could be other microtasks scheduled.
If we now yield back from waitFor we restore the REACT_IS_ACT_ENVIRONMENT. However, the tester awaits the waitFor and before the runtime yields back to the event loop, it needs (by spec) to drain the scheduled microtasks. This may include state updates which now run without REACT_IS_ACT_ENVIRONMENT which triggers warning.

How:

Drain microtask queue before resolving waitFor call.

Checklist:

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 8, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bfd1db7:

Sandbox Source
React Configuration
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #1137 (bfd1db7) into alpha (1934bf2) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             alpha     #1137   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          181       192   +11     
  Branches        34        38    +4     
=========================================
+ Hits           181       192   +11     
Flag Coverage Δ
experimental 100.00% <100.00%> (ø)
latest 100.00% <100.00%> (ø)
next 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pure.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@MatanBobi MatanBobi left a comment

Choose a reason for hiding this comment

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

Looks good! :)

src/__tests__/end-to-end.js Show resolved Hide resolved
src/__tests__/end-to-end.js Outdated Show resolved Hide resolved
@robin-drexler
Copy link

👋 @eps1lon, curious would this PR also address #1051 or this is a different issue? 🤔

Thanks a lot!

@fastndead
Copy link

Hey! is there an ETA for this fix? We've ran into the bunch not wrapped in act() warnings after React 18/RLT 13 update, too many to spot -fix, but these changes actually reduce the amount of failed test by 65%, so I'm really looking forward to this being released

@heath-pack
Copy link

Would love to get this in soon to try and reduce the noise from our specs, then I can see if we have any "actual" act warnings left

@diogoredin
Copy link

Hey @eps1lon, any chance this going to get merged soon? We are also facing some unexpected failing tests & act warnings after moving to RTL 13 (using the legacy root still). Thank you for your work 🙏

@eps1lon
Copy link
Member Author

eps1lon commented Feb 15, 2023

Hey! is there an ETA for this fix? We've ran into the bunch not wrapped in act() warnings after React 18/RLT 13 update, too many to spot -fix, but these changes actually reduce the amount of failed test by 65%, so I'm really looking forward to this being released

This is very helpful information, thank you!

We have a new major planned that will include some small breaking changes from /dom. I was hesitant to merge this PR but including it in a new major makes it a lot safer to land.

I'll clean this up, test on some larger repos and check how it performs.

@eps1lon eps1lon changed the base branch from main to alpha February 15, 2023 20:13
@eps1lon eps1lon force-pushed the fix/nanotasks-troll branch from fb22f8a to d0da54c Compare February 16, 2023 10:24
@eps1lon eps1lon changed the title fix: Prevent "missing act" warning for in-flight promises fix: Prevent "missing act" warning for queued microtasks Feb 16, 2023
eps1lon added a commit to eps1lon/act-warning-mcve that referenced this pull request Feb 16, 2023
@eps1lon eps1lon marked this pull request as ready for review February 16, 2023 17:05
@eps1lon eps1lon merged commit f78839b into testing-library:alpha Feb 16, 2023
@eps1lon eps1lon deleted the fix/nanotasks-troll branch February 16, 2023 22:46
if (jestFakeTimersAreEnabled()) {
jest.advanceTimersByTime(0)
}
})
Copy link

Choose a reason for hiding this comment

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

An observation: if I comment out this new await code that drains the microtask queue, all the end-to-end.js tests are still passing. Both macrotask and microtask, in all timer variants.

Seems the tests would need to be amended somehow to really reproduce the bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests in #1215

@ak-beam
Copy link

ak-beam commented Aug 10, 2023

@eps1lon Would it be possible to backport this to the 12.x release series for codebases still on React 17?

@MatanBobi
Copy link
Member

@ak-beam this fix was only merged to alpha and isn't merged to master yet.
Having said that, I don't think this will be backported because it uses IS_REACT_ACT_ENVIRONMENT which is only available in React 18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants