fix(breaking): use real timers internally to fix awaiting with fake timers #568

mikeduminy · 2020-10-08T08:17:25Z

Summary

If you use Timer Mocks in your tests and waitFor then you cannot await this as the timers internal to waitFor would be replaced by the mock timers and thus never resolve. This PR borrows techniques from wait-for-expect and @testing-library/dom to detect if fake timers are in-use, switch back to real timers for a specific call, then re-instate the fake timers using the timer implementation that was present before.

This implementation also ensures that any mocks of Date or Date.now will not affect waitFor.

Outstanding work: update the docs if this lands.

fixes #506

Test plan

Added multiple tests to ensure this works. Unfortunately there are problems when awaiting a promise that uses 'modern' timers (the default in jest 27) and the react-native preset polyfilling Promise.

NOTE: jest.useFakeTimers('modern') and awaiting a promise in the context of react-native will timeout

There is a way to fix this, but it requires patching the @jest/fake-timers library as described here.

mikeduminy · 2020-10-08T08:42:10Z

src/flushMicroTasks.js

@@ -1,5 +1,6 @@
 // @flow
 import { printDeprecationWarning } from './helpers/errors';
+import { setImmediate } from './helpers/getTimerFuncs';


Not sure about this change. Should promises always be flushed through real timers? Where does this flush even get called?

@thymikee do you know more about where this flush is used and if it should always use real timers?

mikeduminy · 2020-10-20T07:57:20Z

@thymikee and other maintainers, what do you think of this PR?

thymikee · 2020-10-20T08:42:33Z

Still haven't found enough time to review this properly. Please bare with us a while longer 🙈

mikeduminy · 2020-10-20T10:18:23Z

No problem! I appreciate the demands of reviewing code like this :)

mdjastrzebski

I'm not a specialist on intricacies of jest timers so I just toss my two cents.

src/__tests__/timerUtils.js

mdjastrzebski · 2020-10-21T14:36:57Z

src/waitFor.js

+  let interval = options?.interval ?? DEFAULT_INTERVAL;
+
+  if (interval < 1) interval = 1;
+  const maxTries = Math.ceil(timeout / interval);


maxTries maybe wrong, since interval is not guaranted to be exact, for small intervals the delay might be larger due to evenc cycle time.

delay Optional
The time, in milliseconds (thousandths of a second), the timer should wait before the specified function or code is executed. If this parameter is omitted, a value of 0 is used, meaning execute "immediately", or more accurately, the next event cycle. Note that in either case, the actual delay may be longer than intended; see Reasons for delays longer than specified below.

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout

Not sure if that affects your code logic.

It's an interesting point but I don't think it affects things. There is a slight delay when scheduling timeouts and this may lead to a case where a timeout is reached later than it should have been, but this should only happen if the interval is 0 or otherwise very low. We could set a minimum interval?

Other notes:

it's a good idea to not rely on Date since this can also be mocked

this approach is what RNTL v6 used

in the future this can be improved by borrowing an approach from dom-testing-library

Maybe we should adopt wait-for from DOM Testing Library, what do you think? https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for.js
Interesting thing is that inside they advance fake timers, which we currently miss. There may be some other small edge-cases they handle already. Sounds like a sane thing to do

Changed waitFor to be based on DOM Testing Library's implementation

pke · 2020-11-19T16:29:11Z

@michaelduminy thanks for this patch, I really hope this solves the rest of the problems the modern fake timers introduced. Could you please solve the potential merge conflicts so this PR could be further reviewed and merged?

GoMino · 2020-11-24T13:13:48Z

UP 🙄

flextremedev · 2020-12-11T12:09:29Z

Any updates on when this gets reviewed?

thymikee · 2020-12-11T15:07:06Z

Hey, it's a bit of a pain to get through it. Bare with us please. I've got @jaworek and @suezzo working on it, hopefully they can provide some findings here :)

thymikee · 2020-12-14T16:46:45Z

src/__tests__/waitForElementToBeRemoved.test.js

@@ -130,30 +131,21 @@ test('waits with custom interval', async () => {
  expect(mockFn).toHaveBeenCalledTimes(4);
 });

-test('works with legacy fake timers', async () => {
-  jest.useFakeTimers('legacy');
+test.each([TimerMode.Default, TimerMode.Legacy])(


we should now test all timers legacy and modern, the same, as they're supposed to work the same

Unfortunately this does not work right now because the RN jest setup patches global.Promise which breaks modern timers.

This PR would need to land first.

An alternative approach is to manually capture and restore the original global.Promise as seen here. WDYT?

I went with the alternative approach

src/__tests__/timerUtils.js

src/__tests__/timers.test.js

src/__tests__/waitFor.test.js

export other timer helpers, adjust tests and docs

mikeduminy · 2021-02-08T11:53:36Z

Update: tests are all passing using legacy and modern fake timers.

Please have another look @thymikee @jaworek @suezzo 🙏

thymikee · 2021-02-08T11:55:47Z

Thanks! Booking some time to have a look at this soon

Trancever · 2021-02-17T10:23:05Z

I can confirm this PR works in my case. I published a fork to the npm, so feel free to try it out - https://www.npmjs.com/package/react-native-testing-library-fork

mikeduminy · 2021-02-26T10:47:05Z

How do we feel about merging this? 😀

thymikee · 2021-02-26T10:59:10Z

@michaelduminy I feel very strongly to do it! :D I'm just swamped with work recently and want to have a proper look at this before merging. Targeting next week for the work on this 🙂

thymikee · 2021-03-02T11:09:54Z

src/__tests__/waitFor.test.js

@@ -76,39 +77,63 @@ test('waits for element with custom interval', async () => {
    // suppress
  }

-  expect(mockFn).toHaveBeenCalledTimes(3);
+  expect(mockFn).toHaveBeenCalledTimes(2);


this is a change in behavior

sbalay · 2021-03-02T12:48:33Z

jest/preset.js

@@ -0,0 +1,10 @@
+const reactNativePreset = require('react-native/jest-preset');


Is this being exposed as a jest preset of @testing-library/react-native or is it just used locally for the tests in this codebase? If exposed, it should be documented somewhere

Yup, will do in a followup

thymikee

This makes sense. I'm good with the changes that make us vendor implementation of wait-for from the dom-testing-library. I think it's safe to assume this change may be breaking for some users, so I'd like to include it in next major. We also need to expose the preset and document usage when Jest flips the switch

thymikee · 2021-03-02T12:49:42Z

Thank you @michaelduminy, great contribution!

thymikee · 2021-03-02T14:15:03Z

This is released under https://github.com/callstack/react-native-testing-library/releases/tag/v8.0.0-rc.0

flextremedev · 2021-03-02T14:28:41Z

Good job! Works like a charm.

mikeduminy · 2021-03-02T22:54:45Z

Awesome that this landed, thanks for reviewing and cleaning up things!

mikeduminy · 2021-05-24T10:09:56Z

@thymikee Are we waiting for other PRs to land before releasing version 8 (non-RC)?

thymikee · 2021-05-24T10:30:49Z

Not really, I should promote the latest version to stable v8. Will do it once I find some time soon

henrymoulton · 2021-08-04T00:09:08Z

@thymikee anything we can do to help get v8 out as stable? Happy to test the RC but currently test suite isn't that large.

thymikee · 2021-08-04T08:03:03Z

You can treat the RC as a stable version at this point. I will promote it soon, but not sure when exactly.

henrymoulton · 2021-08-04T13:26:38Z

thanks, appreciate you and your work on this awesome library!

mikeduminy commented Oct 8, 2020

View reviewed changes

mikeduminy mentioned this pull request Oct 12, 2020

Migration guide from @testing-library/react-native is missing some breaking changes #477

Closed

thymikee requested review from mdjastrzebski and cross19xx and removed request for mdjastrzebski October 21, 2020 10:42

mdjastrzebski reviewed Oct 21, 2020

View reviewed changes

sbalay mentioned this pull request Oct 21, 2020

Error thrown - Warning: You called act(async () => ...) without await. #379

Closed

thymikee mentioned this pull request Nov 19, 2020

fix: modern jest timers #397

Merged

thymikee reviewed Dec 14, 2020

View reviewed changes

src/__tests__/timerUtils.js Outdated Show resolved Hide resolved

thymikee reviewed Dec 14, 2020

View reviewed changes

src/__tests__/timers.test.js Outdated Show resolved Hide resolved

thymikee reviewed Dec 14, 2020

View reviewed changes

src/__tests__/waitFor.test.js Outdated Show resolved Hide resolved

mikeduminy added 10 commits January 20, 2021 22:00

capture global timers, use them in waitFor, tests+

3e9e373

use real timers in flushMicroTasks

b88aa2b

test to show changes with modern timer mocks

08351f7

use enum-like object for timer modes

f45177e

fix tests

c433020

remove default timer mode, add flow types

1928ec5

export other timer helpers, adjust tests and docs

base waitFor on RTL implementation

90b6115

[jest] Capture and restore global promise

ba2d439

[waitFor] Simulate intervals when using fake timers

84280fb

[tests] Fix waitForElementToBeRemoved

5086038

thymikee reviewed Mar 2, 2021

View reviewed changes

thymikee changed the title ~~Use real timers internally~~ feat(breaking): use real timers internally; decrease timeout to 1s Mar 2, 2021

cleanups; use more specific global

1567942

thymikee changed the title ~~feat(breaking): use real timers internally; decrease timeout to 1s~~ fix(breaking): use real timers internally to fix awaiting with fake timers Mar 2, 2021

rename globals to match others

8c88d6a

sbalay reviewed Mar 2, 2021

View reviewed changes

thymikee approved these changes Mar 2, 2021

View reviewed changes

thymikee merged commit 2ccec3c into callstack:master Mar 2, 2021

thymikee mentioned this pull request Mar 2, 2021

feat: expose custom Jest preset restoring global Promise #695

Merged

0916dhkim mentioned this pull request Mar 22, 2021

Tests migration and subscription message fixes hyochan/hackatalk#366

Merged

5 tasks

jonpepler mentioned this pull request May 27, 2021

findBy doesn't find and waitFor doesn't wait #736

Closed

		@@ -0,0 +1,10 @@
		const reactNativePreset = require('react-native/jest-preset');

fix(breaking): use real timers internally to fix awaiting with fake timers #568

fix(breaking): use real timers internally to fix awaiting with fake timers #568

Conversation

mikeduminy commented Oct 8, 2020 • edited Loading

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeduminy commented Oct 20, 2020

thymikee commented Oct 20, 2020

mikeduminy commented Oct 20, 2020

mdjastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pke commented Nov 19, 2020

GoMino commented Nov 24, 2020

flextremedev commented Dec 11, 2020

thymikee commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeduminy commented Feb 8, 2021

thymikee commented Feb 8, 2021

Trancever commented Feb 17, 2021

mikeduminy commented Feb 26, 2021

thymikee commented Feb 26, 2021

Choose a reason for hiding this comment

sbalay Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

thymikee commented Mar 2, 2021

thymikee commented Mar 2, 2021

flextremedev commented Mar 2, 2021

mikeduminy commented Mar 2, 2021

mikeduminy commented May 24, 2021

thymikee commented May 24, 2021 • edited Loading

henrymoulton commented Aug 4, 2021

thymikee commented Aug 4, 2021

henrymoulton commented Aug 4, 2021

mikeduminy commented Oct 8, 2020 •

edited

Loading

sbalay Mar 2, 2021 •

edited

Loading

thymikee commented May 24, 2021 •

edited

Loading