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
806 stars 107 forks source link

`clock.next()` only fires a single timer, even if multiple timers scheduled at the same time #250

Closed SimenB closed 4 years ago

SimenB commented 5 years ago

Jest recently merged a PR adding advanceTimersToNextTimer: https://github.com/facebook/jest/pull/8713. I assumed the lolex implementation of this would just be calling clock.next(), but that only fires a single timer.

What did you expect to happen?

That if I advance time to the first timer, all timers scheduled at that moment in time fires. Otherwise, code triggers that never would in real code.

What actually happens

Only the first timer triggered. As can be seen in the above linked example, doing

clock.next();
clock.tick(0);

behaves the way I want. So I'm able to work around it, but I'm not sure if the current behavior in Lolex is correct?

How to reproduce See linked runkit

benjamingr commented 5 years ago

The current behavior is what I would expect - timers are guaranteed to be in order - what would you prefer?

SimenB commented 5 years ago

I think I'd prefer all timers at that point in time to fire. So next() would basically be

  1. Find the next scheduled timer
  2. advance time to that point using .tick

rather than

  1. Find the next scheduled timer
  2. advance time there without firing any timers
  3. run the timer we found in step 1.

So essentially next wouldn't mean "next scheduled timer", it would mean "next point in time when timers are scheduled".


Did that make sense? I'm confusing myself with my wording 😛

benjamingr commented 5 years ago

How would you only run one timer in that case?

SimenB commented 5 years ago

You wouldn't, just like real code wouldn't be able to interject between to real setTimeout calls

SimenB commented 5 years ago

E.g.

let foo = false;
let bar = true;
setTimeout(() => {
  foo = true
}, 10)
setTimeout(() => {
  bar = false
}, 10)

const foobar = foo && bar

foobar === true // this should never be observable
benjamingr commented 5 years ago

Right but real code might want to assert that the first timer runs before the second:

let foo = false;
let bar = true;
// file1
setTimeout(() => {
  bar = true;
  foo = true
}, 10);
// file 2 
setTimeout(() => {
  bar = false
}, 10);

How do I assert that the first timer ran before the second here?

SimenB commented 5 years ago

You wouldn't be able to. I'd get on my high horse and say you're trying to test unobservable implementation details, and being able to do so illustrates a leaky abstraction.

However, since this is the real world and we should be more pragmatic, I agree that this is a valid use case that would not be possible anymore if we go with my suggestion. I'm arguing that's a good thing, but I fully understand that not everybody will agree with me (and again, I can work around this easily in my own code).

Happy to close this out, it's been a good discussion

benjamingr commented 5 years ago

Well, I am not yet convinced one way or another 😅

SimenB commented 5 years ago

@fatso83 thoughts?

fatso83 commented 5 years ago

@SimenB I'm trusting you guys :-)

But seriously, I think the current implementation is the expected behaviour for that method.

Still, I'm a pleaser; couldn't we add a runNextBatchOfTimers that fulfills the other scenario? I mean, you already have the implementation :-)

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SimenB commented 4 years ago

Since it's not immediately obvious the current approach is wrong/confusing I'll just close this. Thanks!