GeezerVison

Review requested:

• @nodejs/test_runner

I don't think Date.now should be passed as a param while enabling timers as it's not a timer.

IMHO In this first version Date.now should advance in time when some .tick call happens

cc @nodejs/test_runner

I don't think Date.now should be passed as a param while enabling timers as it's not a timer.

I think it's a timer in the sense that's an API to manipulate or count time in general. But I can see your point too, it can be confusing, I took as an inspiration the last PR and Sinon's fake timers API, this last one also mocks not only the now but the entire Date constructor.

But IMO it kinda makes sense to be here, I would expect an API to manipulate date to be in the fake timers implementation as an user.

IMHO In this first version Date.now should advance in time when some .tick call happens

Regarding this, it's already happening in this implementation, tick is advancing the #now property which is returned by Date.now but setting a time with setTime is not triggering tick, this is a problem that I'll address in the upcoming commits.

Mocking Date.now() without mocking new Date() will be a massive footgun.

Mocking Date.now() without mocking new Date() will be a massive footgun.

Yeah I thought so later on, it wouldn't make a lot of sense 😄 thanks for clarifying! Will add this to the next steps

a83b9f9 introduces a problem and a question. When runAll is called, the easiest way to implement is to do like @ErickWendel did and tick to Infinity, however, this will break the Date mock as Infinity is not a valid epoch.

From here I think we have two options:

1. After running runAll return the epoch value (#now) to what it was before the tick, this will run all the timers but won't advance the date (current implementation, easiest one)
2. Calculate the amount of time required to run all the timeouts (which is basically picking up the last one from the priority queue) and tick the time to that, this way the new Date object will resemble to have run all the ticks required for the timers to execute.

Any thoughts?

a83b9f9 introduces a problem and a question. When runAll is called, the easiest way to implement is to do like @ErickWendel did and tick to Infinity, however, this will break the Date mock as Infinity is not a valid epoch.

From here I think we have two options:

1. After running runAll return the epoch value (#now) to what it was before the tick, this will run all the timers but won't advance the date (current implementation, easiest one)
2. Calculate the amount of time required to run all the timeouts (which is basically picking up the last one from the priority queue) and tick the time to that, this way the new Date object will resemble to have run all the ticks required for the timers to execute.

Any thoughts?

I think runAll shouldn't affect Date.now as it's just releasing all pending operations so option 1 is the best IMHO

@ErickWendel: I think runAll shouldn't affect Date.now as it's just releasing all pending operations so option 1 is the best IMHO

There is a third option here which is to have a property in the enable method which could be called shouldAdvanceTimeOnRunAll that would implement the second option as well. This would cover for use cases where running the timers and checking the date are actually interconnected, for example, testing banking systems that could rely on a timestamp being after another to order events in a timeline after being called from a cron or something

I don't understand the rationale not to progress the clock date whenever it should happen (e.g by 20ms if a 20ms setTimeout delayed has run)?

I think this was the sinon/jest behavior and I don't think anyone ever asked for anything different?

I was under the impression that when Date was mocked, time never advanced except manually by the user.

I was under the impression that when Date was mocked, time never advanced except manually by the user.

As far as I remember from our code in fake-timers we always progress Date/performance.now when we progress timers.

I was under the impression that when Date was mocked, time never advanced except manually by the user.

As far as I remember from our code in fake-timers we always progress Date/performance.now when we progress timers.

Alright! I think I'll work on them today up to the end and we can implement that version, I also think it's the best solution, then we can have a base v1 ready :)

@ljharb @ErickWendel @benjamingr I think I'm done with this version 😄 if you could please review it 🚀

Other than my comments LGTM

@khaosdoctor I've retried it one more time, if it fails again I suggest you rebase on top of main.

@RafaelGSS Now it seems to be good to go!

CI: https://ci.nodejs.org/job/node-test-pull-request/54858/

CI: https://ci.nodejs.org/job/node-test-pull-request/54871/

CI: https://ci.nodejs.org/job/node-test-pull-request/54965/

CI: https://ci.nodejs.org/job/node-test-pull-request/55050/

CI: https://ci.nodejs.org/job/node-test-pull-request/55076/

Merging @anonrig changes in the tests that includes some of them (including the ones failing here) as flaky to check if this will pass

CI: https://ci.nodejs.org/job/node-test-pull-request/55134/

Landed in 45a0b15

OMG FINALLY ⭐

Thanks @anonrig for the flaky PRs!