Tweak waitFor to not bottleneck async tasks

[nstepien]

What: I'm addressing issues with waitFor bottlenecking async tasks and ultimately timing out in some cases. Ref: #820 (comment)

Why: The MutationObserver callback can fire quite often, and if the check is slow/expensive, it can prevent async tasks from being run to completion in a timely manner, ultimately leading to waitFor timing out. Similarly, setInterval can queue a new task before the check is complete, so now the "interval" will be between the end of the previous check and the beginning of the next check, instead of between the start of two checks. The "interval" is where async work can actually run, so it's wise to relinquish the main thread to non-test code during that time.

How: Added a delay in the MutationObserver callback, swapped setInterval for setTimeout

Checklist:

• Documentation added to the
  docs site
• Tests
• TypeScript definitions updated
• Ready to be merged
  

There are 2 non-covered lines now, let me know if you want me to try and add more tests to cover them.

[codesandbox-ci]

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 a4ca576:

Sandbox	Source
react-testing-library-examples	Configuration

[nstepien]

jest (2nd run results)

Before

Test Suites: 1 failed, 51 passed, 52 total
Tests:       1 failed, 1 skipped, 223 passed, 225 total
Snapshots:   0 total
Time:        91.298 s

After

Test Suites: 52 passed, 52 total
Tests:       1 skipped, 224 passed, 225 total
Snapshots:   0 total
Time:        90.435 s

jest --runInBand

Before

Test Suites: 1 failed, 51 passed, 52 total
Tests:       1 failed, 1 skipped, 223 passed, 225 total
Snapshots:   0 total
Time:        374.919 s

After

Test Suites: 52 passed, 52 total
Tests:       1 skipped, 224 passed, 225 total
Snapshots:   0 total
Time:        380.614 s

Conclusion: I get fewer flaky tests in our internal tests without needing configure({ asyncUtilTimeout: 2500 }), and the performance is about the same overall.

[eps1lon]

Thanks for the work. Some lines aren't covered with tests though.

[nstepien]

@eps1lon Coverage should be good now.
The CI is failing, but it seems to be unrelated. (see kentcdodds/kcd-scripts#221)

[codecov]

Codecov Report

Merging #1031 (a4ca576) into main (fb6f2d5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1031   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          919       930   +11     
  Branches       283       287    +4     
=========================================
+ Hits           919       930   +11     

Flag	Coverage Δ
node-12	100.00% <100.00%> (ø)
node-14	100.00% <100.00%> (ø)
node-16.9.1	100.00% <100.00%> (ø)

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

Impacted Files	Coverage Δ
src/wait-for.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6f2d5...a4ca576. Read the comment docs.

[eps1lon]

Shouldn't this be greater than interval to illustrate that the callback always completed before another one is started?

[nstepien]

The test fails as expected when this line is removed 🤷‍♂️:

dom-testing-library/src/wait-for.js

Line 140 in 45830f5

if (promiseStatus === 'pending') return

When using fake timers, we use setTimeout(r, 0) in a while loop, which is why this test works:

dom-testing-library/src/wait-for.js

Line 92 in 45830f5

setTimeout(r, 0)

I can still increase the timer if you'd prefer.

[eps1lon]

I don't understand how this is related. What are you testing and why is it related to flushing microtasks? My original statement would still apply to real timer usage.

[nstepien]

I had to add this test to reach 100% testing coverage

dom-testing-library/src/wait-for.js

Line 140 in 45830f5

if (promiseStatus === 'pending') return

☝️ this line isn't covered anymore without this test.

I've tweaked the test now, please take another look.

[eps1lon]

The MutationObserver callback can fire quite often, and if the check is slow/expensive, it can prevent async tasks from being run to completion in a timely manner, ultimately leading to waitFor timing out.

There's currently no test for this. If this change relates to #1031, then you want to do some expensive work in waitFor (e.g. just running a while loop for N milliseconds where N is greater than the interval).

Also "avoid running the next check when the current check is still pending" is already passing on main. I don't see how this test is testing what it says it does.