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

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

Closed
SimenB opened this issue Aug 16, 2019 · 12 comments
Labels

Comments

@SimenB
Copy link
Member

SimenB commented Aug 16, 2019

Jest recently merged a PR adding advanceTimersToNextTimer: jestjs/jest#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
Copy link
Member

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

@SimenB
Copy link
Member Author

SimenB commented Aug 16, 2019

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 timer_s_ are scheduled".


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

@benjamingr
Copy link
Member

How would you only run one timer in that case?

@SimenB
Copy link
Member Author

SimenB commented Aug 16, 2019

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

@SimenB
Copy link
Member Author

SimenB commented Aug 16, 2019

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

benjamingr commented Aug 16, 2019

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

SimenB commented Aug 16, 2019

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

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

@SimenB
Copy link
Member Author

SimenB commented Aug 16, 2019

@fatso83 thoughts?

@fatso83
Copy link
Contributor

fatso83 commented Aug 16, 2019

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

stale bot commented Dec 8, 2019

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.

@stale stale bot added the stale label Dec 8, 2019
@SimenB SimenB closed this as completed Dec 8, 2019
@SimenB
Copy link
Member Author

SimenB commented Dec 8, 2019

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

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

No branches or pull requests

3 participants