test_runner: Add Date to the supported mock APIs by khaosdoctor · Pull Request #48638 · nodejs/node

khaosdoctor · 2023-07-02T19:29:31Z

This builds on top of @ErickWendel's #47775, I saw the next steps would be to implement the mock timers for Date.now (and thus, the Date object) and performance.now.

This PR implements the Date.now mock, I'll also work on performance.now on another PR to make it simpler to review. This one includes the Docs already updated and the added tests.

This heavily builds on Sinon's Fake Timers for the base edge cases

To-Do

Refactor the code to make setTime actually call the tick method and pass the time
Make the initial time be 0 or accept an instance of Date, or a specific number to make it's implementation be the same as fake-timers
Mock the Date object completely
Allow enable to accept multiple overloads

Next iterations

Mock performance.now
Mock process.hrtime

New `MockTimers` API

// All that was before, with no changesMockTimers.reset()// clean up to the original stateMockTimers.tick(100)// advance in time by 100msMockTimers.runAll()// release all timers// Implementation changesMockTimers.enable({timersToEnable: ['setInterval','setTimeout','Date'],now: 1000})// enable fake timers with the initial timer set (optional)// New methodsMockTimers.setTime(100)// sets Date.now to the desired value

It's also possible to omit the initial parameter and pass on only the initial epoch, which will enable all timers with that epoch set:

// All that was before, with no changesMockTimers.reset()// clean up to the original stateMockTimers.tick(100)// advance in time by 100msMockTimers.runAll()// release all timers// Implementation changesMockTimers.enable({now: 1000})// enable fake timers with the initial timer set (optional)// New methodsMockTimers.setTime(100)// sets Date.now to the desired value

Lastly, you can omit all parameters to enable all timers at the epoch 0:

// All that was before, with no changesMockTimers.reset()// clean up to the original stateMockTimers.tick(100)// advance in time by 100msMockTimers.runAll()// release all timers// Implementation changesMockTimers.enable()// New methodsMockTimers.setTime(100)// sets Date.now to the desired value

Example usage

importassertfrom'node:assert';import{test}from'node:test';test('mocks Date.now to whatever value the user sets',(context)=>{constnow=Date.now()console.log(now)// not mocked, will print correct timestampcontext.mock.timers.enable({apis: ['Date']});// This will be roughly true. Just to take it as exampleassert.strictEqual(now,Date.now())// if not set, will take the value of Date.now() as initial valuecontext.mock.timers.setTime(1000)assert.strictEqual(Date.now(),1000)// Date.now is 1000});

importassertfrom'node:assert';import{test}from'node:test';test('mocks Date.now to whatever value the user sets',(context)=>{constnow=Date.now()console.log(now)// not mocked, will print correct timestampcontext.mock.timers.enable({apis: ['Date'],now: 1000});// This will be roughly true. Just to take it as exampleassert.strictEqual(Date.now(),1000)// Date.now is 1000 due to being set in enable()});

All the remaining APIs are the same

nodejs-github-bot · 2023-07-02T19:29:35Z

Review requested:

@nodejs/test_runner

ErickWendel · 2023-07-03T11:33:52Z

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

khaosdoctor · 2023-07-03T16:49:21Z

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.

ljharb · 2023-07-03T17:58:47Z

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

khaosdoctor · 2023-07-03T19:43:21Z

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

khaosdoctor · 2023-07-06T23:20:43Z

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:

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)
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?

ljharb · 2023-07-07T02:38:21Z

+ this.#now = 0;
+ } else if (this.#isValidDateWithGetTime(args[0])) {
+ // First argument is the initial time as Date
+ this.#now = args[0].getTime();


do we mean to call .getTime() on the argument, or should this instead be:
Suggested change
this.#now =args[0].getTime();
this.#now =DatePrototypeGetTime(args[0]);

It's the intended one, if the first argument is an instance of Date then we will call the getTime method on it to get the timestamp

right but if it's an actual Date object - which may not be instanceof Date - you don't want to rely on the presence of Date.prototype.getTime at runtime.

ErickWendel · 2023-07-07T20:08:54Z

 #isEnabled = false;
 #currentTimer = 1;
- #now = DateNow();
+ #now = 0;


#now should be DateNow unless it's changed by .setTime

But that differs from the implementation from fake-timers. If the intention is to be close to that API then we wouldn't be able to start it as DateNow, however, I'm not against that too. I think it's worth a discussion with @nodejs/test_runner
On my side I think the pros is that you can start the Date object without actually setting anything on it, as opposed to having to set the time at any given new fake timer instance because otherwise you'd have Jan 1, 1970. Which also can be a pro to some people as it's more explicit to what time is the initial time.

ErickWendel · 2023-07-07T20:20:51Z

- p.then(common.mustCall((result) => {
- assert.ok(result);
- }));
+ p.then(


ErickWendel · 2023-07-07T21:11:58Z

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:
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)
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

khaosdoctor · 2023-07-07T22:16:27Z

@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

benjamingr · 2023-07-09T17:21:06Z

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?

ljharb · 2023-07-09T22:32:15Z

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

benjamingr · 2023-07-12T15:15:00Z

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.

khaosdoctor · 2023-07-12T16:36:35Z

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 :)

khaosdoctor · 2023-07-12T22:38:02Z

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

benjamingr · 2023-07-14T05:30:13Z

Other than my comments LGTM

RafaelGSS · 2023-10-13T22:21:34Z

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

khaosdoctor · 2023-10-14T14:58:42Z

@RafaelGSS Now it seems to be good to go!

rozzilla

Cool!

nodejs-github-bot · 2023-10-16T01:38:06Z

Commit Queue failed

- Loading data for nodejs/node/pull/48638
✔ Done loading data for nodejs/node/pull/48638
----------------------------------- PR info ------------------------------------
Title test_runner: Add Date to the supported mock APIs (#48638)
Author Lucas Santos (@khaosdoctor)
Branch khaosdoctor:test_runner/introduce_improve_fake_timers -> nodejs:main
Labels semver-minor, notable-change, author ready, needs-ci, commit-queue-squash, test_runner
Commits 3
- test_runner: add Date to the supported mock APIs
- Apply suggestions from code review
- Apply suggestions from code review
Committers 1
- Lucas Santos PR-URL: https://github.com/nodejs/node/pull/48638
Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow Reviewed-By: Erick Wendel Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48638
Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow Reviewed-By: Erick Wendel Reviewed-By: Rafael Gonzaga --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - test_runner: add Date to the supported mock APIs
⚠ - Apply suggestions from code review
⚠ - Apply suggestions from code review
ℹ This PR was created on Sun, 02 Jul 2023 19:29:31 GMT
✔ Approvals: 4
✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/48638#pullrequestreview-1655247395
✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/48638#pullrequestreview-1630117150
✔ - Erick Wendel (@erickwendel): https://github.com/nodejs/node/pull/48638#pullrequestreview-1536004400
✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/48638#pullrequestreview-1665791940
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2023-10-10T12:43:52Z: https://ci.nodejs.org/job/node-test-pull-request/54632/
⚠ Commits were pushed after the last Full PR CI run:
⚠ - test_runner: add Date to the supported mock APIs
⚠ - Apply suggestions from code review
⚠ - Apply suggestions from code review
- Querying data for job/node-test-pull-request/54632/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6527901651

aduh95

RSLGTM

nodejs-github-bot · 2023-10-16T18:35:50Z

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

nodejs-github-bot · 2023-10-17T05:38:07Z

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

nodejs-github-bot · 2023-10-18T19:02:31Z

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

nodejs-github-bot · 2023-10-20T14:34:42Z

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

nodejs-github-bot · 2023-10-21T03:00:59Z

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

signed-off-by: Lucas Santos <lhs.santoss@gmail.com>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

khaosdoctor · 2023-10-22T18:48:17Z

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

nodejs-github-bot · 2023-10-23T02:26:52Z

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

nodejs-github-bot · 2023-10-23T11:23:14Z

Landed in 45a0b15

khaosdoctor · 2023-10-23T12:47:30Z

OMG FINALLY ⭐

khaosdoctor · 2023-10-23T12:47:56Z

Thanks @anonrig for the flaky PRs!

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 2, 2023

khaosdoctor changed the title ~~test_runner: implement first iteration of date.now~~test_runner: implement Date.now mock as a MockTimerJul 2, 2023

khaosdoctor commented Jul 2, 2023
View reviewed changes

Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated

khaosdoctor mentioned this pull request Jul 5, 2023
test_runner: implement first iteration of date.now #48637
Closed

ljharb reviewed Jul 7, 2023
View reviewed changes

ErickWendel reviewed Jul 7, 2023
View reviewed changes

Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated

ljharb reviewed Jul 7, 2023
View reviewed changes

Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated
Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated

khaosdoctor requested review from ErickWendel and ljharb July 12, 2023 22:38

benjamingr reviewed Jul 13, 2023
View reviewed changes

Comment threaddoc/api/test.md Outdated

benjamingr reviewed Jul 13, 2023
View reviewed changes

Comment threaddoc/api/test.md Outdated

benjamingr reviewed Jul 13, 2023
View reviewed changes

Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated

ErickWendel reviewed Jul 13, 2023
View reviewed changes

Comment threaddoc/api/test.md Outdated

MoLow reviewed Jul 14, 2023
View reviewed changes

Comment threadlib/internal/test_runner/mock/mock_timers.js Outdated

rozzilla reviewed Oct 15, 2023
View reviewed changes

aduh95 reviewed Oct 16, 2023
View reviewed changes

Comment threaddoc/api/test.md Outdated
Comment threaddoc/api/test.md Outdated
Comment threaddoc/api/test.md Outdated
Comment threaddoc/api/test.md Outdated
Comment threaddoc/api/test.md Outdated

aduh95 approved these changes Oct 16, 2023
View reviewed changes

RafaelGSS approved these changes Oct 17, 2023
View reviewed changes

This was referenced Oct 19, 2023
CI Reliability 2023-10-19 nodejs/reliability#691
Open
CI Reliability 2023-10-20 nodejs/reliability#692
Open

github-actionsBot mentioned this pull request Oct 21, 2023
CI Reliability 2023-10-21 nodejs/reliability#693
Open
24 tasks

github-actionsBot mentioned this pull request Oct 22, 2023
CI Reliability 2023-10-22 nodejs/reliability#694
Open
29 tasks

khaosdoctorand others added 4 commits October 22, 2023 20:45

test_runner: add Date to the supported mock APIs
52176c9
signed-off-by: Lucas Santos <lhs.santoss@gmail.com>

Apply suggestions from code review
a794944
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Apply suggestions from code review
ea24c0a
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Apply suggestions from code review
8991402
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

github-actionsBot mentioned this pull request Oct 23, 2023
CI Reliability 2023-10-23 nodejs/reliability#695
Open
26 tasks

RafaelGSS approved these changes Oct 23, 2023
View reviewed changes

github-actionsBot mentioned this pull request Oct 24, 2023
CI Reliability 2023-10-24 nodejs/reliability#696
Open
27 tasks

targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged

UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged

	this.#now =args[0].getTime();
	this.#now =DatePrototypeGetTime(args[0]);

Uh oh!

Uh oh!

Conversation

khaosdoctor commented Jul 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To-Do

Next iterations

New MockTimers API

Example usage

Uh oh!

nodejs-github-bot commented Jul 2, 2023

Uh oh!

Uh oh!

ErickWendel commented Jul 3, 2023

Uh oh!

khaosdoctor commented Jul 3, 2023

Uh oh!

ljharb commented Jul 3, 2023

Uh oh!

khaosdoctor commented Jul 3, 2023

Uh oh!

khaosdoctor commented Jul 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ljharbJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

khaosdoctorJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

ljharbJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ErickWendelJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

khaosdoctorJul 7, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ErickWendelJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ErickWendel commented Jul 7, 2023

Uh oh!

Uh oh!

Uh oh!

khaosdoctor commented Jul 7, 2023

Uh oh!

benjamingr commented Jul 9, 2023

Uh oh!

ljharb commented Jul 9, 2023

Uh oh!

benjamingr commented Jul 12, 2023

Uh oh!

khaosdoctor commented Jul 12, 2023

Uh oh!

khaosdoctor commented Jul 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjamingr commented Jul 14, 2023

khaosdoctor commented Jul 2, 2023•
edited
Loading

New `MockTimers` API

khaosdoctorJul 7, 2023•
edited
Loading