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

* refactor: rewrite test so that it is more explicit

* refactor: make test faster by decreasing both interval and timeout

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

* test: fix typo in test comment

* tests: increase a bit interval so test is more robust and make it more explicit

* tests: some improvements from review

* Update expect comment following review suggestion

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

Co-authored-by: pierrezimmermann <pierrez@nam.tech>
Co-authored-by: Maciej Jastrzebski <mdjastrzebski@gmail.com>

Author: 3 people

src/__tests__/waitFor.test.tsx

@@ -204,6 +204,47 @@ test.each([false, true])(
   }
 );
 
+const blockThread = (timeToBlockThread: number, legacyFakeTimers: boolean) => {
+  jest.useRealTimers();
+  let end = Date.now() + timeToBlockThread;
+
+  while (Date.now() < end) {
+    // do nothing
+  }
+
+  jest.useFakeTimers({ legacyFakeTimers });
+};
+
+test.each([true, false])(
+  'it should not depend on real time when using fake timers (legacyFakeTimers = %s)',
+  async (legacyFakeTimers) => {
+    jest.useFakeTimers({ legacyFakeTimers });
+    const WAIT_FOR_INTERVAL = 20;
+    const WAIT_FOR_TIMEOUT = WAIT_FOR_INTERVAL * 5;
+
+    const mockErrorFn = jest.fn(() => {
+      // Wait 2 times interval so that check time is longer than interval
+      blockThread(WAIT_FOR_INTERVAL * 2, legacyFakeTimers);
+      throw new Error('test');
+    });
+
+    await expect(
+      async () =>
+        await waitFor(mockErrorFn, {
+          timeout: WAIT_FOR_TIMEOUT,
+          interval: WAIT_FOR_INTERVAL,
+        })
+    ).rejects.toThrow();
+
+    // Verify that the `waitFor` callback has been called the expected number of times
+    // (timeout / interval + 1), so it confirms that the real duration of callback did not
+    // cause the real clock timeout when running using fake timers.
+    expect(mockErrorFn).toHaveBeenCalledTimes(
+      WAIT_FOR_TIMEOUT / WAIT_FOR_INTERVAL + 1
+    );
+  }
+);
+
 test.each([false, true])(
   'awaiting something that succeeds before timeout works with fake timers (legacyFakeTimers = %s)',
   async (legacyFakeTimers) => {

src/waitFor.ts

@@ -38,7 +38,7 @@ function waitForInternal<T>(
     let finished = false;
     let promiseStatus = 'idle';
 
-    const overallTimeoutTimer = setTimeout(handleTimeout, timeout);
+    let overallTimeoutTimer: NodeJS.Timeout | null = null;
 
     const usingFakeTimers = jestFakeTimersAreEnabled();
 
@@ -64,6 +64,7 @@ function waitForInternal<T>(
 
         // when fake timers are used we want to simulate the interval time passing
         if (fakeTimeRemaining <= 0) {
+          handleTimeout();
           return;
         } else {
           fakeTimeRemaining -= interval;
@@ -90,6 +91,7 @@ function waitForInternal<T>(
         await new Promise((resolve) => setImmediate(resolve));
       }
     } else {
+      overallTimeoutTimer = setTimeout(handleTimeout, timeout);
       intervalId = setInterval(checkRealTimersCallback, interval);
       checkExpectation();
     }
@@ -98,7 +100,9 @@ function waitForInternal<T>(
       done: { type: 'result'; result: T } | { type: 'error'; error: unknown }
     ) {
       finished = true;
-      clearTimeout(overallTimeoutTimer);
+      if (overallTimeoutTimer) {
+        clearTimeout(overallTimeoutTimer);
+      }
 
       if (!usingFakeTimers) {
         clearInterval(intervalId);