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 changes
MockTimers.reset()  // clean up to the original state
MockTimers.tick(100) // advance in time by 100ms
MockTimers.runAll() // release all timers

// Implementation changes
MockTimers.enable({ timersToEnable: ['setInterval', 'setTimeout', 'Date' ], now: 1000 }) // enable fake timers with the initial timer set (optional)

// New methods
MockTimers.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 changes
MockTimers.reset()  // clean up to the original state
MockTimers.tick(100) // advance in time by 100ms
MockTimers.runAll() // release all timers

// Implementation changes
MockTimers.enable({ now: 1000 }) // enable fake timers with the initial timer set (optional)

// New methods
MockTimers.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 changes
MockTimers.reset()  // clean up to the original state
MockTimers.tick(100) // advance in time by 100ms
MockTimers.runAll() // release all timers

// Implementation changes
MockTimers.enable()

// New methods
MockTimers.setTime(100) // sets Date.now to the desired value

Example usage

import assert from 'node:assert';
import { test } from 'node:test';

test('mocks Date.now to whatever value the user sets', (context) => {
  const now = Date.now()
  console.log(now) // not mocked, will print correct timestamp

  context.mock.timers.enable({ apis: ['Date'] });

  // This will be roughly true. Just to take it as example
  assert.strictEqual(now, Date.now()) // if not set, will take the value of Date.now() as initial value
  context.mock.timers.setTime(1000)
  assert.strictEqual(Date.now(), 1000) // Date.now is 1000
});

import assert from 'node:assert';
import { test } from 'node:test';

test('mocks Date.now to whatever value the user sets', (context) => {
  const now = Date.now()
  console.log(now) // not mocked, will print correct timestamp

  context.mock.timers.enable({ apis: ['Date'], now: 1000 });

  // This will be roughly true. Just to take it as example
  assert.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

lib/internal/test_runner/mock/mock_timers.js

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?

lib/internal/test_runner/mock/mock_timers.js

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

lib/internal/test_runner/mock/mock_timers.js

+      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.

lib/internal/test_runner/mock/mock_timers.js

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

lib/internal/test_runner/mock/mock_timers.js

  #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.

lib/internal/test_runner/mock/mock_timers.js

test/parallel/test-runner-mock-timers.js

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

test/parallel/test-runner-mock-timers.js

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


lib/internal/test_runner/mock/mock_timers.js

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

lib/internal/test_runner/mock/mock_timers.js

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 🚀

doc/api/test.md

lib/internal/test_runner/mock/mock_timers.js

doc/api/test.md

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

Other than my comments LGTM

lib/internal/test_runner/mock/mock_timers.js

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

doc/api/test.md

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 <[email protected]>

Co-authored-by: Antoine du Hamel <[email protected]>

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 MockTimer Jul 2, 2023

khaosdoctor commented Jul 2, 2023

View reviewed changes

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

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

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

ljharb reviewed Jul 7, 2023

View reviewed changes

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

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

benjamingr reviewed Jul 13, 2023

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

benjamingr reviewed Jul 13, 2023

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

benjamingr reviewed Jul 13, 2023

View reviewed changes

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

ErickWendel reviewed Jul 13, 2023

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

MoLow reviewed Jul 14, 2023

View reviewed changes

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

rozzilla reviewed Oct 15, 2023

View reviewed changes

aduh95 reviewed Oct 16, 2023

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

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-actions bot mentioned this pull request Oct 21, 2023

CI Reliability 2023-10-21 nodejs/reliability#693

Open

24 tasks

github-actions bot mentioned this pull request Oct 22, 2023

CI Reliability 2023-10-22 nodejs/reliability#694

Open

29 tasks

khaosdoctor and others added 4 commits October 22, 2023 20:45

test_runner: add Date to the supported mock APIs

52176c9

signed-off-by: Lucas Santos <[email protected]>

Apply suggestions from code review

a794944

Co-authored-by: Antoine du Hamel <[email protected]>

Apply suggestions from code review

ea24c0a

Co-authored-by: Antoine du Hamel <[email protected]>

Apply suggestions from code review

8991402

Co-authored-by: Antoine du Hamel <[email protected]>

github-actions bot 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-actions bot 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!

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!

ljharb Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

khaosdoctor Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

ljharb Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ErickWendel Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

khaosdoctor Jul 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!

ErickWendel Jul 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

Uh oh!

khaosdoctor commented Jul 2, 2023 •

edited

Loading

New `MockTimers` API

khaosdoctor Jul 7, 2023 •

edited

Loading