testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.27k stars 467 forks source link

findBy* no longer waiting when used with jest fake timers #988

Open deini opened 3 years ago

deini commented 3 years ago

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:

eps1lon commented 3 years ago

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?

deini commented 3 years ago

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.

eps1lon commented 3 years ago

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?

deini commented 3 years ago

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.

eps1lon commented 3 years ago

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).

deini commented 3 years ago

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

eps1lon commented 3 years ago

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.

kentcdodds commented 3 years ago

Cc @kettanaito

weyert commented 3 years ago

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) ?

eps1lon commented 3 years ago

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.

timdeschryver commented 3 years ago

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

weyert commented 3 years ago

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?

kettanaito commented 3 years ago

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.

weyert commented 3 years ago

I will try to make a repo as promised :)

eps1lon commented 3 years ago

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.

weyert commented 3 years ago

@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?

mjetek commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

william-hotmart commented 3 years ago

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

olegKusov commented 3 years ago

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

mc0 commented 3 years ago

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

kettanaito commented 3 years ago

@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.

berndartmueller commented 2 years ago

Is there any working/recommended solution for this issue?

rbrewington commented 2 years ago

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.

prashantjainlacework commented 2 years ago

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?

adnan-sheikh commented 2 years ago

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?

rbrewington commented 2 years ago

@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.

adnan-sheikh commented 2 years ago

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

rbrewington commented 2 years ago

@adnan-sheikh have you tried extending the timeout? I know I had issues with this when I updated the testing-library b/c newer versions set the timeout to 1 second. What happens if you try

await waitForElementToBeRemoved(() => screen.queryByTestId("loader"), { timeout: 5000 });
adnan-sheikh commented 2 years ago

@rbrewington setting timeout option is having no effect

adnan-sheikh commented 2 years ago

The only way this timeout is not happening is by not using jest.useFakeTimers(), but then the test fails again, not here but at the end where there is a notification component, which takes 2 or 3 seconds to get removed. To get rid of the latter timeout error, I need to set timeout explicitly for notification component removal.

And then for some tests, I need to do jest.setTimeout(10000) for tests to pass

rbrewington commented 2 years ago

Yes, that tracks with my own experience. I have had to increase the timeout globally for all async utils. In src/setupTests.ts:

import { configure } from '@testing-library/dom';
configure({ asyncUtilTimeout: 5000 });
adnan-sheikh commented 2 years ago

If only I can use jest.useFakeTimers(), test times can be improved

morinted commented 2 years ago

My current workaround is to downgrade to @testing-library/react to v11.

EmreErdogan commented 2 years ago

My workaround for waitForElementToBeRemoved is wrapping it like this:

import { waitForElementToBeRemoved as originalWaitForElementToBeRemoved } from "@testing-library/react";

export const waitForElementToBeRemoved = async (callback, options) => {
  jest.runOnlyPendingTimers();
  jest.useRealTimers();
  await originalWaitForElementToBeRemoved(callback, options);
  jest.useFakeTimers();
};

Then whenever you need waitForElementToBeRemoved just import your own waitForElementToBeRemoved function instead of importing from testing library.

I believe the same technique can be used for other async utils like waitFor and findBy*.

Hope this helps.

Morta1 commented 1 year ago

Any one had any luck solving this issue?

domarmstrong commented 1 year ago

Swap out window.fetch for another fetch implementation like https://www.npmjs.com/package/node-fetch in your global setup