testing-library / dom-testing-library

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

Allow fake timers other than jest #987

Open IanVS opened 3 years ago

IanVS commented 3 years ago

Describe the feature you'd like:

This is an follow-on to https://github.com/testing-library/dom-testing-library/issues/939. The request is to support fake timers other than jest. For instance, jest uses @sinonjs/fake-timers under the hood, and yet it's not possible to use @sinonjs/fake-timers directly with testing-library without jest. I would like to be able to use testing-library with @sinonjs/fake-timers without jest.

Suggested implementation:

@eps1lon had a good suggestion in https://github.com/testing-library/dom-testing-library/issues/939#issuecomment-830771708 to decouple testing-library from jest timers, and @MatanBobi suggested doing so in a way that can be set up in the configure step instead of requiring wrappers.

Describe alternatives you've considered:

I have tried to manually advance the clock with clock.tick(), and it works but causes act warnings.

Teachability, Documentation, Adoption, Migration Strategy:

I'm not certain what, if anything, will need to be done in user land to make this work. Ideally, it could be handled under-the-hood within this library, but I'm not sure how that could be done in a robust way.

eps1lon commented 3 years ago

For instance, jest uses @sinonjs/fake-timers under the hood, and yet it's not possible to use @sinonjs/fake-timers directly with testing-library.

The reference to jest is not relevant here. We do support jest's modern fake timers (sinonjs) and their own legacy timers.

What I'd like to see is the option to pass clock adapter that we use internally. Then people can write adapters for all sorts of fake timer libraries and we just continue supporting jest.

IanVS commented 3 years ago

Thanks @eps1lon. That's essentially the approach you laid out in the comment I linked to, right? So the goal would be jest would continue work "out of the box" via the detection mechanisms already in place, and if anyone wants to use something else, they would call domTestingLibrary.useFakeTimers() and pass in a timing adapter made for the library they want to use? That seems pretty strightforward and flexible, and would give me what I need. I can work on a PR if ya'll are happy with the approach.

eps1lon commented 3 years ago

they would call domTestingLibrary.useFakeTimers() and pass in a timing adapter made for the library they want to use?

That was my initial plan, yes. Not sure about the API yet. And I think we may want to cleanup waitFor a bit before. I don't think we need to branch between fake and real timers. Instead we need to setup the clock adapter just once. For the real clock the adapter would just contain no-ops.

Though we may want to setup a custom "flush microtask" method. But that's not important for a first iteration of this feature.

IanVS commented 3 years ago

And I think we may want to cleanup waitFor a bit before.

Can you talk a bit more about this? Is that work being tracked anywhere, in a separate issue maybe?

jwalton commented 3 years ago

I've actually been using @sinonjs/fake-timers with @testing-library/react@11.1.1, @testing-library/user-event@13.1.9, jest@26.6.3, and sinon@9.0.3 quite successfully - I had no idea I wasn't supposed to. Upgrading to the latest @testing-library with the new @testing-library/dom makes a whole bunch of my tests fail and spews out tons of warnings about jest.useFakeTimers().

eps1lon commented 3 years ago

Can you talk a bit more about this? Is that work being tracked anywhere, in a separate issue maybe?

It's not blocking this issue.

evoyy commented 3 years ago

@jwalton that sounds similar to my situation in https://github.com/testing-library/dom-testing-library/issues/992. I had to rework my tests to make sure I restore all timers before calling waitFor. Unfortunately this means I have also lost some testing granularity (as explained in the issue).

mjetek commented 3 years ago

I think setting different fake timers implementation shouldn't be in the scope of this library. It would make more sense for jest to allow different fake timers implementations. But I think it would be useful to have option whether fake or real timers should be used with waitFor functions and maybe real timers should be used by default (as jsdom environment always use real timers and this library is all about interacting with jsdom).

Argument for using fake timers with async utilities was that we shouldn't mix timers, but because jsdom is always using real timers we will always be dealing with both real and fake timers being used.

jwalton commented 3 years ago

It would make more sense for jest to allow different fake timers implementations.

What if you want to use this library with mocha? Or with some other non-jest test runner?

mjetek commented 3 years ago

if someone is using mocha and mock timers with sinon is this not supported? I think testing-library should work with any test runner + fake timers combination but should not require any extra api to do it.

IanVS commented 3 years ago

this library is all about interacting with jsdom.

Is this true? I'm using this library to interact with the real DOM via web-test-runner, and it seems to work well, aside from the issues I've had with fake timers.

evoyy commented 3 years ago

this library is all about interacting with jsdom.

Is this true? I'm using this library to interact with the real DOM via web-test-runner, and it seems to work well, aside from the issues I've had with fake timers.

I run my tests in Chrome using Karma and I never had any problems. 600 tests in 10K lines of code.

jwalton commented 3 years ago

According to this comment Jest fake timers are the only one supported.

I'm using jest + sinon timers, and everything was working fine for me up until the latest release.

This particular combination is a problem. dom-testing-library does some custom checking to see if jest timers are enabled (which will come back true in this case, even though jest timers are not enabled, because sinon will create a setTimeout.clock - any other fake timer library that creates a setTimeout.clock would have this same problem). This library will then make calls into jest fake timers, which will raise lots of warnings from jest since fake timers are not enabled.

KamiKillertO commented 2 years ago

Any news on that topic? I'm personally using vitest, which has an API compatible with the jest one. But, because it's not jest I had to tweak the example from here in order to make it works.


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

test('Component render', async () => {
  const { container } = render(<MyComponent/>)

   // I have to switch back to real timer otherwhise the await will never resolve
  vi.runOnlyPendingTimers()
  vi.useRealTimers()

  const heading = await screen.findByRole('heading', { level: 1 })
  expect(heading).toHaveTextContent('xxx')
}
ChristopherChudzicki commented 2 years ago

@KamiKillertO I am also use vitest and ran into this issue. This has worked for me:

// for vitest
// see See https://sinonjs.org/releases/latest/fake-timers/
vi.useFakeTimers({ shouldAdvanceTime: true });

// for jest
// see https://jestjs.io/docs/jest-object#jestusefaketimersfaketimersconfig 
jest.useFakeTimers({ advanceTimers: true })

I rather agree with @mjetek's sentiment that

I think setting different fake timers implementation shouldn't be in the scope of this library.

I guess this enables real time advancement for all timers, whereas it probably would be ideal to only have real-time for waitFor. But this is pretty good, IMO. One thing that would make sense to put in testing-library is some error message / warning if waitFor is used with fake timers. Something like:

<Current waitFor error message>
You appear to be using fake timers. Ensure that your fake timers also advance real time in order for `waitFor` to work properly.
KamiKillertO commented 2 years ago

@ChristopherChudzicki Thanks. I've tested with vi.useFakeTimers({ shouldAdvanceTime: true }); and it's working perfectly.