sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
804 stars 106 forks source link

Sinon fails to uninstall clock when using jest.fakeTimers() #410

Closed Niryo closed 2 years ago

Niryo commented 2 years ago

I am using both Sinon clock and jest.useFakeTimers for legacy reasons, and I am getting an error while trying to uninstall sinon clock: TypeError: _global.setInterval is not a function after enabling jest.useFakeTimers() during some test. I know that it's a bit fucked up to use both sinon and jest.useFakeTimers, but anyways I think that sinon should handle this situation gracefully.

What did you expect to happen? Sinon should be able to uninstall clock even after using jest.fakeTimers What actually happens Sinon throws exception when trying to uninstall clock after using jest.fakeTimers How to reproduce

  1. install sinon clock:
    clock = FakeTimers.install({
    shouldAdvanceTime: true,
    });
  2. use jest.fakeTimers()
  3. try to uninstall clock.
  4. TypeError: _global.setInterval is not a function
SimenB commented 2 years ago

Note that Jest ships with @sinonjs/fake-timers if you use "modern" fake timers (default in v27), and mixing them is not supported. You should use one or the other.

Niryo commented 2 years ago

I know, the issue is that I am working on a big code base and I want to add the fake timers to global beforeEach, but we have lot's of legacy tests that still use jest.fakeTimers. I know it's not a good practice to mix those two, but maybe we could still handle the collision gracefully.

fatso83 commented 2 years ago

Hi, @Niryo . If you want us to have a look at this you need to provide us with a better reproduction case, such as the versions used. It is unclear if you are using "old" Jest timers or the new, for instance. The optimal thing would be if you created a small repo that had a package.json, a code file that demonstrated the issue and a NPM test script that would then fail when invoked. If you linked to that repo we would know when we had fixed the issue, as we would then have a good reproduction case. The more effort you put into this, the less work we have to do to understand the issue and the better your chances of having this fixed becomes :-)

P.S. You might see that once you have a simple way of exposing the issue, the path to fixing it becomes much shorter for yourself as well, as you understand the issue better, and maybe you can see the fix in the fake-timers.js yourself. Talking from experience 😉

Niryo commented 2 years ago

tested on jest 26.6.3: (it actually works with jest 27, i'll check if I can update our legacy tests to jest 27) minimal repo: https://github.com/Niryo/sinon-fake-timers-bug/tree/main

benjamingr commented 2 years ago

I don't think fake-timers makes any guarantees when other code that mocks timers is run.

I think the best we should do is warn when fake-timers are overridden but I guess "PRs welcome" :)

@Niryo just guessing by the name but if you are local and want to write a PR feel free to reach out on FB/whatsapp and I will happily guide you on how to fix this.

Niryo commented 2 years ago

@benjamingr thanks:) i'll ping you if i'll have time to create a PR. But actually as long as sinon works with the latest jest version, it makes this issue quite low priority I guess. I leave it to you guys if you want to keep this issue open or not. thanks!

fatso83 commented 2 years ago

Was thinking the same. If this works with Jest 27 and not with Jest 26, it's probably good. My guess is that Jest 26 imports another version of fake-timers (I miss the lolex name), whereas Jest 27 uses the same and somehow accesses the same global or something.

In any case, closing, but I can have a new look if it should resurface later! Just reopen.

SimenB commented 2 years ago

26 (by default) uses its own implementation and the one using Lolex is opt-in via jest.useFakeTimere('modern'). In v27 "modern" is the default. Might also be a mismatch in version of Lolex of course