Philosophy in regards to Jest fake timers

[slightlytyler]

I'm exploring deprecating flushPromises in my own project (not using react-test-library) in favor of a wait approach based on this library. The one flaw I see is that this library fails when using fake timers. How do you suggest users approach this problem?

[kentcdodds]

This is a great question. I honestly have no idea :-(

[lgandecki]

@slightlytyler Good point.

We could use runWithRealTimers -
https://github.com/facebook/jest/blob/master/packages/jest-util/src/fake_timers.js#L296 -
inside this package, after detecting jest, to make sure we don't use the mocks. But, looks like jest internally is rewriting the fakeTimers (
jestjs/jest#5171 ) , and they might work completely different soon. :( One of the things they talk about getting rid of is this very function. Not sure what to do to be honest.

Could you show us some code where you feel that using BOTH wait and jest fake timers might make sense? It might help us to visualize this problem better.

[lgandecki]

One thing that comes to my mind now - on the import of wait-for-expect we could do a side-effect like:

const realSetTimeout = setTimeout

and then use that when actually calling the function, so we wouldn't care if anything overwrote the globals meanwhile.

@slightlytyler If you are able to show us some code examples - breaking repro would be amazing :-) I can try to get it to pass

[slightlytyler]

I can probably add a failing case for you. A trivial example exposes the problem. Consider a search box that debounces input before calling a handler => which results in promises added to the queue => with an eventual rerender when the data resolves. We'll need to wait not only for the promises to resolve (what we need wait-for-expect for) but also for the debounce timeout to occur. Using fake timers here improves the speed and reliability of the test as we don't care if the debounce is 300ms or 30s.

[lgandecki]

Makes sense!
Yeah, if you are able to add a breaking test that would be fantastic. I should be able to put some time into this today.
Thanks!

[kentcdodds]

This is tricky! Personally I'd rather not encode jest-specific stuff into this package. I have an idea of something to do if you could provide a simple example that I could update (you may even be able to use codesandbox).

[slightlytyler]

Doesn't look like codesandbox supports fake timers but here y'all go https://github.com/slightlytyler/wait-for-expect-fake-timers

[lgandecki]

Thanks so much! I will try to squeeze in a bit time today to try to make it pass. I agree that it would be best to not make anything jest-specific here.
I'm also curious about what Kent has in mind. :-)

[lgandecki]

@slightlytyler
I'm not sure if I understand this correctly but this test fails even if you don't use waitForExpect

jest.useFakeTimers();

describe("SlowSearchBox", () => {
  it("should show results", async () => {
    const { getByTestId } = render(<SlowSearchBox />);
    Simulate.change(getByTestId("input"), {
      target: {
        value: "baco"
      }
    });
    jest.runAllTimers();
  });
});

Looks like the jest fake timers doesn't play nice with debounce, when I changed your code to:

let debounce;
(..)
  search = () => {
    clearTimeout(debounce)
    debounce = setTimeout(
      () => this.setState({results: findEmojis(this.state.query)}),
      2000
    );
  }

it clicked and worked, but I didn't need to use waitForExpect:

    jest.runAllTimers();
    expect(getByTestId("result-list")).toBeInTheDOM()
    expect(getByTestId("result-item")).toBeInTheDOM()
    expect(getByTestId("result-list")).toHaveTextContent("🥓")

worked fine.
When I put them inside waitForExpect they stopped working, because, without calling runAllTimers, the setTimeout inside wait-for-expect will never finish, so I was getting jest timeout.

Adding:

const setTimeout = global.setTimeout;

at the top of wait-for-expect fixes this particular problem. And it's not really jest specific. It would take care of making this lib compatible with any library that messes up with the setTimeout.

What do you think @kentcdodds ?

[kentcdodds]

Interesting. I was going to look into and suggest possibly calling jest.runAllTimers() between each waitForExpect call. But it looks like that's not necessary.

I'm pretty happy with that solution (const setTimeout = global.setTimeout), except I'd want to make sure it can run in the browser as well, I think that's an important use case for react-testing-library.

That said, as far as I'm concerned we can make the assumption that there will be a global window object (react-testing-library mounts objects to the DOM so it's required to use jsdom in node). Does this still work with const setTimeout = window.setTimeout?

[slightlytyler]

@lgandecki you're right I messed up the example (thanks lodash.debounce). Fixed the example to actually illustrate the problem

[slightlytyler]

@kentcdodds can confirm const setTimeout = window.setTimeout works as well as const setTimeout = global.setTimeout

[kentcdodds]

Cool (also I'm a little confused why this works... I would expect Jest to fake out all timers regardless of whether it's on global or window).

So this isn't really my lib, but what I'd recommend is adding this to the top of the file here:

// Used to avoid using Jest's fake timers.
// See https://github.com/TheBrainFamily/wait-for-expect/issues/4 for more info
const setTimeout = (typeof window !== 'undefined' ? window : global).setTimeout

[lgandecki]

@kentcdodds what happens is - jest overwrites the global setTimeout, but if you assign it to a local variable before that then your local one stays unchanged.
I’m happy with the proposed code, wanna make a PR? :-)
Since we are using jest for testing here I think we could have a test for that as well. You can just make a PR without a test and I can add it later (I’m off till Sunday)

[kentcdodds]

As this isn't impacting me personally at the moment, I'll leave this for @slightlytyler to implement :)