Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow fake timers other than jest #987

Open
IanVS opened this issue Jun 23, 2021 · 16 comments
Open

Allow fake timers other than jest #987

IanVS opened this issue Jun 23, 2021 · 16 comments
Labels
enhancement New feature or request

Comments

@IanVS
Copy link
Contributor

IanVS commented Jun 23, 2021

Describe the feature you'd like:

This is an follow-on to #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 #939 (comment) 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
Copy link
Member

eps1lon commented Jun 23, 2021

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.

@eps1lon eps1lon added the enhancement New feature or request label Jun 23, 2021
@IanVS
Copy link
Contributor Author

IanVS commented Jun 23, 2021

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
Copy link
Member

eps1lon commented Jun 23, 2021

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
Copy link
Contributor Author

IanVS commented Jun 30, 2021

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
Copy link

jwalton commented Jul 12, 2021

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
Copy link
Member

eps1lon commented Jul 12, 2021

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
Copy link

evoyy commented Jul 12, 2021

@jwalton that sounds similar to my situation in #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
Copy link

mjetek commented Aug 1, 2021

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
Copy link

jwalton commented Aug 1, 2021

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
Copy link

mjetek commented Aug 1, 2021

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
Copy link
Contributor Author

IanVS commented Aug 2, 2021

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
Copy link

evoyy commented Aug 2, 2021

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
Copy link

jwalton commented Aug 3, 2021

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
Copy link

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
Copy link

ChristopherChudzicki commented Oct 4, 2022

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants