fix: do not use a real timeout in waitfor when using fake timers #1177

pierrezimmermannbam · 2022-10-16T17:16:00Z

Summary

Use a timeout in waitFor only when using realTimers and timeout with fake timers only after fakeTimeRemaining is over

Test plan

I added a test case that initially failed. I compute fibonaci suite so that it takes some time to perform the checks (it must take longer than interval for this bug to be visible)

This shouldn't be a breaking change, all tests that previously passed should still do because we're not performing any check after fakeTimeRemaining is over anyway

mdjastrzebski · 2022-10-17T08:47:23Z

src/__tests__/waitFor.test.tsx

+const fibonaci = (n: number): number => {
+  if (n === 0 || n === 1) {
+    return 1;
+  }
+
+  return fibonaci(n - 1) + fibonaci(n - 2);
+};


Could we replace this with something both more controllable and explicit:

Suggested change

const fibonaci = (n: number): number => {

if (n === 0 || n === 1) {

return 1;

}

return fibonaci(n - 1) + fibonaci(n - 2);

};

function blockThread(milliseconds)

{

var end = Date.now() + milliseconds;

while (Date.now() < end) {

// Intentionally do nothing

};

}

Very good idea, it's way more explicit that way! I just had to switch to real timers when calling blockThread so that it works

mdjastrzebski · 2022-10-17T08:48:43Z

src/__tests__/waitFor.test.tsx

+test.each([false, true])(
+  'it should not depend on real time when using fake timers (legacyFakeTimers = %s)',
+  async () => {
+    jest.useFakeTimers({ legacyFakeTimers: false });


This test does not seem to use each parameter. Pls remove each param or apply it to legacyFakeTimers

Oh my bad, fixed it

src/__tests__/waitFor.test.tsx

mdjastrzebski

@pierrezimmermannbam I've reviewed the code the best I could, but I do not feel an expert in fake timers.

The changes in the waitFor.ts file seem reasonable. Though I felt safer just sticking to the DTL code which we copied, but that might be just my poor understanding of fake timers ;-)

I've left some comments & suggestions about test code.

@thymikee pls take a look at this PR, you seem to know much more about fake timers than I.

pierrezimmermannbam · 2022-10-17T17:23:57Z

@mdjastrzebski thanks for the review ! I believe I implemented all the changes you requested

src/__tests__/waitFor.test.tsx

mdjastrzebski · 2022-10-17T21:20:32Z

src/__tests__/waitFor.test.tsx

+
+    const mockErrorFn = jest.fn(() => {
+      // Wait 5 seconds so that check time is longer than interval
+      blockThread(5, legacyFakeTimers);


I would wait a little bit longer to make the test more robust using the exact value might cause some race conditions

Suggested change

blockThread(5, legacyFakeTimers);

blockThread(8, legacyFakeTimers);

Actually I'm not using the exact value, the aim is that we block the thread for a time longer than the interval, but I'll make it a bit more explicit

mdjastrzebski · 2022-10-17T21:23:36Z

src/__tests__/waitFor.test.tsx

+          timeout: 6,
+          interval: 2,


You seem to be using very tight intervals, which could be fragile. Let use some interval in ~20-100 range (maybe 50 ms)

I don't see how using tight intervals could be fragile since we're using fake timers but maybe I'm thinking too much based in the current implementation so I increased it to 20

src/__tests__/waitFor.test.tsx

mdjastrzebski · 2022-10-17T21:25:58Z

src/__tests__/waitFor.test.tsx

+    // Verify that even though time to perform check is longer than interval
+    // test won't timeout until number of checks * interval >= timeout
+    // ie fake timers have been advanced by timeout when waitfor rejects


Suggested change

// Verify that even though time to perform check is longer than interval

// test won't timeout until number of checks * interval >= timeout

// ie fake timers have been advanced by timeout when waitfor rejects

// Verify that even though time to perform check is longer than interval

// test won't timeout until number of checks * interval >= timeout

// i.e. fake timers have been advanced (at least?) by timeout when waitFor rejects

mdjastrzebski

I think I start to understand this a bit more. I've left some comments for the testing code.

…e explicit

pierrezimmermannbam · 2022-10-19T07:02:41Z

@mdjastrzebski thanks again for your feedbacks, I treated all your comments

mdjastrzebski

Looks good, I think I finally grasped what is happening both in the code and in the test.

Good job @pierrezimmermannbam for catching it. I've suggested a rewrite to the expect comment to clarify the intent. Pls use/modify as you find it suitable.

src/__tests__/waitFor.test.tsx

mdjastrzebski · 2022-10-19T20:58:31Z

@thymikee @AugustinLF pls take a look at this PR, as I do not feel expert on fake timers.

Co-authored-by: Maciej Jastrzebski <mdjastrzebski@gmail.com>

AugustinLF · 2022-10-20T08:34:45Z

@mdjastrzebski it looks good to me, but same, I'm not familiar with the timers part of the codebase. I'd suggest releasing this commit alone in a dedicated rc release to be able to do some testing on it, it's the type of changes that could bring some weird errors in some codebases.

mdjastrzebski · 2022-10-20T10:09:59Z

I've asked @thymikee for review, as our "fake timer specialist". That might delay merging for couple of days, but I think it's worth it.

thymikee

Makes sense, thanks!

mdjastrzebski · 2022-10-31T10:25:12Z

@pierrezimmermannbam you might want to submit a similar bug/PR for DOM Testing Library, as they should have the same issue.

fix: do not use a real timeout in waitfor when using fake timers

f957246

pierrezimmermannbam requested review from mdjastrzebski and AugustinLF October 16, 2022 17:34

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

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

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

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

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

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

mdjastrzebski requested changes Oct 17, 2022

View reviewed changes

mdjastrzebski requested a review from thymikee October 17, 2022 09:32

pierrezimmermann added 3 commits October 17, 2022 19:20

refactor: rewrite test so that it is more explicit

e9f383d

refactor: make test faster by decreasing both interval and timeout

3eeefb9

refactor: expect waitFor to throw rather than wrapping in try catch

e6471fe

pierrezimmermannbam force-pushed the fix/waitForFakeTimers branch from 5265fd3 to e6471fe Compare October 17, 2022 17:20

pierrezimmermannbam requested a review from mdjastrzebski October 17, 2022 17:24

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

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

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

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

mdjastrzebski reviewed Oct 17, 2022

View reviewed changes

mdjastrzebski mentioned this pull request Oct 18, 2022

When using fake timers, waitFor still uses a setTimeout with real timers #1176

Closed

pierrezimmermann added 3 commits October 18, 2022 18:29

test: fix typo in test comment

417e2d8

tests: increase a bit interval so test is more robust and make it mor…

eaff7a6

…e explicit

tests: some improvements from review

6686095

pierrezimmermannbam requested a review from mdjastrzebski October 19, 2022 06:59

mdjastrzebski approved these changes Oct 19, 2022

View reviewed changes

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

Update expect comment following review suggestion

7297949

Co-authored-by: Maciej Jastrzebski <mdjastrzebski@gmail.com>

mdjastrzebski added the hacktoberfest-accepted label Oct 31, 2022

thymikee approved these changes Oct 31, 2022

View reviewed changes

thymikee merged commit 380ff32 into callstack:main Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not use a real timeout in waitfor when using fake timers #1177

fix: do not use a real timeout in waitfor when using fake timers #1177

pierrezimmermannbam commented Oct 16, 2022 •

edited

Loading

mdjastrzebski Oct 17, 2022

pierrezimmermannbam Oct 17, 2022

mdjastrzebski Oct 17, 2022

pierrezimmermannbam Oct 17, 2022

mdjastrzebski left a comment

pierrezimmermannbam commented Oct 17, 2022

mdjastrzebski Oct 17, 2022

pierrezimmermannbam Oct 19, 2022

mdjastrzebski Oct 17, 2022

pierrezimmermannbam Oct 19, 2022

mdjastrzebski Oct 17, 2022

mdjastrzebski left a comment

pierrezimmermannbam commented Oct 19, 2022

mdjastrzebski left a comment

mdjastrzebski commented Oct 19, 2022

AugustinLF commented Oct 20, 2022

mdjastrzebski commented Oct 20, 2022

thymikee left a comment

mdjastrzebski commented Oct 31, 2022

	blockThread(5, legacyFakeTimers);
	blockThread(8, legacyFakeTimers);

fix: do not use a real timeout in waitfor when using fake timers #1177

fix: do not use a real timeout in waitfor when using fake timers #1177

Conversation

pierrezimmermannbam commented Oct 16, 2022 • edited Loading

Summary

Test plan

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

mdjastrzebski left a comment

Choose a reason for hiding this comment

pierrezimmermannbam commented Oct 17, 2022

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

mdjastrzebski left a comment

Choose a reason for hiding this comment

pierrezimmermannbam commented Oct 19, 2022

mdjastrzebski left a comment

Choose a reason for hiding this comment

mdjastrzebski commented Oct 19, 2022

AugustinLF commented Oct 20, 2022

mdjastrzebski commented Oct 20, 2022

thymikee left a comment

Choose a reason for hiding this comment

mdjastrzebski commented Oct 31, 2022

pierrezimmermannbam commented Oct 16, 2022 •

edited

Loading