dom-testing-library icon indicating copy to clipboard operation
dom-testing-library copied to clipboard

findBy* no longer waiting when used with jest fake timers

Open deini opened this issue 3 years ago • 39 comments

  • @testing-library/dom version: 8.0.0
  • Testing Framework and version: @testing-library/react v12
  • DOM Environment: jest 27

Relevant code or config:

"jest": {
  "testEnvironment": "jsdom",
  "setupFilesAfterEnv": ["<rootDir>/jest-setup.js"]
}
// jest-setup.js
import '@testing-library/jest-dom'

What you did:

I was trying to use fake timers and findBy*

What happened:

findBy* doesn't wait when using fake timers as it used to.

Reproduction:

Created a repo with two branches, one using v11 of RTL and another one for v12.

v11 branch works fine (dom v7.31.2) v12 branch is experiencing issues (dom v8)

https://github.com/deini/rtl-find/tree/v11 https://github.com/deini/rtl-find/tree/v12

Problem description:

When doing something like:

jest.useFakeTimers();

render(<Button />)

await screen.findByRole('dialog', {}, { timeout: 5000 })

I would expect it to wait for 5 seconds (unless I'm totally wrong 😅), however, seems like fake timers is now messing up with findBy* and it doesn't wait anymore. This happens both in jest 26 and 27 with both legacy and modern fake timers.

Suggested solution:

deini avatar Jun 23 '21 18:06 deini

Thanks for the feedback

I would expect it to wait for 5 seconds (unless I'm totally wrong 😅),

It should wait for 5seconds in the fake clock. It used to wait 5s in the real clock regardless of fake/real timer usage. Maybe your timers would take more than 5s in fake time?

eps1lon avatar Jun 23 '21 19:06 eps1lon

Thanks for the quick reply @eps1lon! I understand the new behavior now but I'm curious in how I could test a couple of things with the new behavior.

I pushed commits to both branches with a different example simulating a simplified "Notifications poller".

This commit on the v11 branch works fine with the old behavior (with jest v26). I'm not sure how to test the same behavior with the new timer changes if I can't use the real clock to wait for the re-renders. I'm probably missing something.

deini avatar Jun 23 '21 21:06 deini

This commit on the v11 branch works fine with the old behavior (with jest v26). I'm not sure how to test the same behavior with the new timer changes if I can't use the real clock to wait for the re-renders. I'm probably missing something.

The test doesn't look like it needs findBy at all since you're already manually advancing the clock. Could you isolate the problem into a cloneable repo so that I can check it out?

eps1lon avatar Jun 24 '21 06:06 eps1lon

Thanks again for the reply. The repo I linked in my previous comments is an isolation of the issue.

Couldn't come up with something smaller to reproduce it.

deini avatar Jun 24 '21 14:06 deini

It seems to me msw is using a different clock. I would expect that if I advance the clock by 200ms and flush the microtask queue any response with a delay <200ms would resolve. But that's not what's happening:


test("renders number of notifications", async () => {
  const fakeTimersEnabled = true;

  if (fakeTimersEnabled) {
    jest.useFakeTimers();
  }

  const server = setupServer(
    rest.get("/api/notifications", (_req, res, ctx) => {
      console.log("GET 1");
      return res(ctx.delay(100), ctx.json({ notifications: ["Foo"] }));
    })
  );
  server.listen();

  let data = null;
  fetch("/api/notifications")
    .then((response) => {
      console.log("response");
      return response.json();
    })
    .then((_data) => {
      data = _data;
    });

  let timeout = 500;
  const interval = 50;

  await new Promise(async (resolve, reject) => {
    let cancelled = false;
    const timeoutError = new Error("timeout");

    setTimeout(() => {
      cancelled = true;
      reject(timeoutError);
    }, timeout);
    setInterval(() => {
      console.log("ping");
      if (data !== null) {
        resolve();
      }
    }, interval);

    if (fakeTimersEnabled) {
      while (!cancelled) {
        jest.advanceTimersByTime(interval);
        await new Promise((resolve) => {
          setTimeout(resolve, 0);
          jest.advanceTimersByTime(0);
        });
      }
    }
  });

  expect(data).not.toEqual(null);
});

This sounds more like an msw issue to me. Or maybe we're disagreeing on whether you should or shouldn't mix clocks (I think you should the clock that's currently available and not mix real/fake clocks).

eps1lon avatar Jun 24 '21 17:06 eps1lon

Thanks for taking a look @eps1lon, closing as it is not actionable on this repo.

deini avatar Jun 24 '21 18:06 deini

Let's keep this open since msw is quite popular and I'd like to ensure compatiblity short-term. I just don't know where to start so some response from their maintainers would be nice.

eps1lon avatar Jun 24 '21 18:06 eps1lon

Cc @kettanaito

kentcdodds avatar Jun 28 '21 00:06 kentcdodds

I think it might be related to an issue I am experiencing. The code for msw is using a setTimeout to implement the timeout.. Could the be conflicting with this library?

You can see the code at: https://github.com/mswjs/msw/blob/c96ffa3cddad355c20062ad49c4b2f9af0cd124b/src/utils/handleRequest.ts#L101

Maybe it needs to check if fake timers are enabled and in those cases run jest.advancedTimerBy(response.delay) ?

weyert avatar Jun 28 '21 00:06 weyert

Maybe it needs to check if fake timers are enabled and in those cases run jest.advancedTimerBy(response.delay) ?

That would already be handled by us. The only important part is that they don't use a different clock e.g. by using a setTimeout they captured during module initialization.

eps1lon avatar Jun 28 '21 07:06 eps1lon

Have you tried running this with msw v.0.30 ? This includes a fix for Jest 27.

timdeschryver avatar Jun 28 '21 08:06 timdeschryver

I will give it a shot. I think when I tried last week the behaviour didn't change much for me. But I am not using Jest 27 yet. Could that be an issue?

weyert avatar Jun 28 '21 14:06 weyert

I think it might be related to an issue I am experiencing. The code for msw is using a setTimeout to implement the timeout.. Could the be conflicting with this library?

There should be no conflicts. Jest stubs the global setTimeout, but MSW is not using that. Instead, it uses a compatible polyfill from the timers package:

https://github.com/mswjs/msw/blob/1da34e4fef13e278043d65ad656875186ac84e90/rollup.config.ts#L132-L134

This was added a while ago so that fake timers in Jest don't interfere with the request handling. I doubt that Jest can technically affect third-party libraries, so I think this isn't the case.

kettanaito avatar Jun 28 '21 14:06 kettanaito

I will try to make a repo as promised :)

weyert avatar Jun 28 '21 14:06 weyert

This was added a while ago so that fake timers in Jest don't interfere with the request handling.

They shouldn't do that. The point of fake timers is that you do want to inferfere with the time. If libraries just use a different clock, fake timers wouldn't do anything.

eps1lon avatar Jun 28 '21 14:06 eps1lon

@eps1lon sorry, I am not following. Do you mean msw should use fake timers too and do my earlier suggested advanceTimerBy? As if you specify a delay or use its standard delay it would be stuck, right?

weyert avatar Jun 29 '21 11:06 weyert

I see the same issues when mocking fetch but using new Response(...) from fetch polyfill (whatwg-fetch). I had this in my code

jest.spyOn(window, 'fetch').mockResolvedValue(new Response(toJSONBlob(mockValue)));

await screen.findByText('something');

And in the component

fetch(url)
.then(r => {
  // This will be executed
  return r => r.json()
})
then(obj => {
  // this will be executed only after test failed on findByText and component got unmounted.
  ...
})   

I managed to fix my test by not using new Response() and instead returning mock like this

{ json: () => Promise.resolve(mockValue), ok: true } 

I did a bit of debugging and I see that in fetch promise returned at line 308 https://github.com/github/fetch/blob/7727e50493eafae9a7005f10f18f81e5bbcbfdd3/fetch.js#L308

return readBlobAsText(this._bodyBlob) resolves after findByText failed.

Any chance that when fake timers are used it blocks FileReaders onload event?https://github.com/github/fetch/blob/7727e50493eafae9a7005f10f18f81e5bbcbfdd3/fetch.js#L169

mjetek avatar Jul 22 '21 11:07 mjetek

I've created repo to demonstrate the issue https://github.com/mjetek/tl-fake-timer with test file https://github.com/mjetek/tl-fake-timer/blob/main/src/App.test.js It does work with @testing-library/react 11 but fails with 12

mjetek avatar Jul 22 '21 20:07 mjetek

I figured out that the issue is caused by use of setImmediate in the jsdom implementation of the FileReader. if I change my test to delay updating state with

await new Promise((resolve) => setImmediate(() => resolve()));

The promise will be resolved only after test failed because of the findBy timing out. But if I do this instead it will work fine.

await new Promise((resolve) => setTimeout(() => resolve(), 0));

I think this is issue with jest, I've created this issue for jest https://github.com/facebook/jest/issues/11692

mjetek avatar Jul 24 '21 10:07 mjetek

I've realised the issue is caused by jsdom still using real timers even though fake timers are used in the test. I don't know if this is expected behaviour. I've closed previous jest issue and opened new one https://github.com/facebook/jest/issues/11710
If jest is going to use real timers for jsdom, I think it perhaps would make sense to stick with the real timers for findBy functions.

mjetek avatar Aug 01 '21 10:08 mjetek

@mjetek Is there some workaround to make the tests works while this problem is not solved?

william-hotmart avatar Sep 28 '21 18:09 william-hotmart

Having same issue. waitFor, findBy and WaitforElementToBeRemoved no longer work with fake timers.

olegKusov avatar Oct 22 '21 13:10 olegKusov

https://github.com/testing-library/dom-testing-library/issues/1072#issuecomment-964107955

The implication here is that msw is the issue. I don't agree given the purpose of waitFor. It doesn't contend that it will use whatever clock is available, and it's not solving the problem it exists to solve today.

The msw change to add NodeJS timers makes sense, given the functionality. Using NodeJS timers in a testing library (even with fake timers installed) makes sense.

Copying my repro from the other issue here: https://codesandbox.io/s/react-testing-library-msw-demo-c7nd2

mc0 avatar Nov 09 '21 13:11 mc0

@mc0, we indeed use the timers module in MSW so fake timers in Jest have no effect over when the response is being sent. I've suggested a way to revert to setTimeout, which should allow us to respect custom timers and no-delay responses at the same time. I'd appreciate your feedback on that.

kettanaito avatar Nov 10 '21 00:11 kettanaito

Is there any working/recommended solution for this issue?

berndartmueller avatar Nov 15 '21 14:11 berndartmueller

I'm having this problem after upgrading to testing-library/react 12.1.2 and jest 27. I'm not using msw, but many of the findBy and waitFor queries in my 200+ unit tests are failing although I've changed nothing about the code or tests.

rbrewington avatar Jan 13 '22 22:01 rbrewington

We are experiencing the same issue. jest fake timers are interfering with waitFor and findBy queries. Both of them make two attempts and the test fails irrespective of the timeout value. Some more data: We provided a 5 seconds timeout and both waitFor and findByXXXX made two retries only. The two retries are something consistent in our environment.

Any updates on this issue?

prashantjainlacework avatar Feb 04 '22 17:02 prashantjainlacework

I am experiencing somewhat similar issue. I am on CRA setup and I upgraded react-scripts to 5.0.0.

Previously I was on: "@testing-library/jest-dom": "^5.11.4", "@testing-library/react": "^11.1.0",

and then I upgraded to: "@testing-library/jest-dom": "^5.16.1", "@testing-library/react": "^12.1.2",

but now my tests are failing,

beforeEach(() => {
  jest.useFakeTimers()
}

afterEach(() => {
  jest.useRealTimers()
}
.
.
.
render(<Component />);
await waitForElementToBeRemoved(() => screen.queryByTestId("loader"));
//   ^? Test is failing here "waitFor... getting timedout"

before upgrading it was passing I'm using MSW (v0.36.3) for mocking.

Note that, If i'm not using fake timers, test passes But, I need fake timers to skip setTimeout on a notification component, as I am waiting for it to be removed at the end of test. Any help?

adnan-sheikh avatar Feb 17 '22 20:02 adnan-sheikh

@adnan-sheikh I have had luck using jest.runOnlyPendingTimers().

I use it during cleanup before using jest.useRealTimers(), and I also use it in places where an asynchronous behavior is happening.

I'm not sure this is "correct", but it works for me.

In your specific example, I would try putting it between your render and await.

rbrewington avatar Feb 17 '22 21:02 rbrewington

@rbrewington Thanks for the reply, I tried that already. But even after using jest.runOnlyPendingTimers(), it is not passing.

adnan-sheikh avatar Feb 17 '22 21:02 adnan-sheikh