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

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

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:

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?

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.

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

same here

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

@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!

Cool!

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

RSLGTM

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/

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

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

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!

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

Comment thread lib/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 thread lib/internal/test_runner/mock/mock_timers.js Outdated

ljharb reviewed Jul 7, 2023

View reviewed changes

Comment thread lib/internal/test_runner/mock/mock_timers.js Outdated

Comment thread lib/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 thread doc/api/test.md Outdated

benjamingr reviewed Jul 13, 2023

View reviewed changes

Comment thread doc/api/test.md Outdated

benjamingr reviewed Jul 13, 2023

View reviewed changes

Comment thread lib/internal/test_runner/mock/mock_timers.js Outdated

ErickWendel reviewed Jul 13, 2023

View reviewed changes

Comment thread doc/api/test.md Outdated

MoLow reviewed Jul 14, 2023

View reviewed changes

Comment thread lib/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 thread doc/api/test.md Outdated

Comment thread doc/api/test.md Outdated

Comment thread doc/api/test.md Outdated

Comment thread doc/api/test.md Outdated

Comment thread doc/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-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 <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-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 = 0;
+                  } else if (this.#isValidDateWithGetTime(args[0])) {
+                    // First argument is the initial time as Date
+                    this.#now = args[0].getTime();

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

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

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

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

Uh oh!

Uh oh!

test_runner: Add Date to the supported mock APIs - #48638

test_runner: Add Date to the supported mock APIs#48638nodejs-github-bot merged 4 commits intonodejs:mainnodejs/node:mainfrom khaosdoctor:test_runner/introduce_improve_fake_timerskhaosdoctor/node:test_runner/introduce_improve_fake_timersCopy head branch name to clipboard

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!

test_runner: Add Date to the supported mock APIs#48638
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
khaosdoctor:test_runner/introduce_improve_fake_timers

khaosdoctor commented Jul 2, 2023 •
edited
Loading

khaosdoctor Jul 7, 2023 •
edited
Loading