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

MockTimers does not mock Timer properties #51701

Closed
mscottnelson opened this issue Feb 8, 2024 · 0 comments · Fixed by #51809
Closed

MockTimers does not mock Timer properties #51701

mscottnelson opened this issue Feb 8, 2024 · 0 comments · Fixed by #51809
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@mscottnelson
Copy link

mscottnelson commented Feb 8, 2024

Version

v20.11.0

Platform

Darwin Kernel Version 23.2.0

Subsystem

node:test

What steps will reproduce the bug?

When performing tests that require the mocking of timers, it is extremely helpful to have all the aspects of timers mocked. Today, MockTimers does not match the API of Timers. This can cause tests to fail simply because the MockTimer will error with normal usage. When testing something that is making heavy usage of timing, this makes the MockTimer API unsuitable for testing timing-related code.

For example:

import { mock } from 'node:test';

const t1 = setInterval(()=>{},100); // returns Timer
t1.unref(); // returns Timer
mock.timers.enable();
const t2 = setInterval(()=>{},100); // returns Object: null prototype
t2.unref(); // Uncaught TypeError

Perhaps the tooling that currently overrides timer behavior could instead be a forwarding Proxy or similar?

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Timers returned by the MockTimer API should match the API of the Timers API.

Expected:

require("node:test").mock.timers.enable();
setInterval(()=>{}).unref();
/**
> Timeout {
  _idleTimeout: 1,
  _idlePrev: [TimersList],
  _idleNext: [TimersList],
  _idleStart: 34,
  _onTimeout: [Function (anonymous)],
  _timerArgs: undefined,
  _repeat: 1,
  _destroyed: false,
  [Symbol(refed)]: false,
  [Symbol(kHasPrimitive)]: false,
  [Symbol(asyncId)]: 2,
  [Symbol(triggerId)]: 1
}
**/

What do you see instead?

Actual:

require("node:test").mock.timers.enable();
setInterval(()=>{}).unref()

TypeError: setInterval(...).unref is not a function

Additional information

No response

@VoltrexKeyva VoltrexKeyva added the test_runner Issues and PRs related to the test runner subsystem. label Feb 8, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 21, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Feb 26, 2024
nodejs-github-bot pushed a commit that referenced this issue Feb 26, 2024
Fixes: #51701
PR-URL: #51809
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
marco-ippolito added a commit that referenced this issue Feb 27, 2024
Fixes: #51701
PR-URL: #51809
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #51701
PR-URL: #51809
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #51701
PR-URL: #51809
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
Fixes: nodejs#51701
PR-URL: nodejs#51809
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants