node test mock timer promisified setTimeout & setInterval don't always return the specified value

[mika-fischer]

Version

v20.5.1

Platform

Microsoft Windows NT 10.0.22621.0 x64

Subsystem

test

What steps will reproduce the bug?

Mocked promisified setTimeout & setInterval don't return falsy values, and instead return numbers.

The reason for setTimeout is here. It works correctly for truthy values, but returns an id for falsy values.

setInterval completely captures the value parameter for its own purposes so that it never resturns the value given to setInterval (see here).

import assert from "node:assert";
import { test } from "node:test";
import timers from "node:timers/promises";

test("original setTimeout", async () => {
  for (const val of [true, false]) {
    const result = await timers.setTimeout(1, val);
    assert.equal(result, val);
  }
});

test("mocked setTimeout", async (context) => {
  context.mock.timers.enable();
  for (const val of [true, false]) {
    const promise = timers.setTimeout(1, val);
    context.mock.timers.tick();
    const result = await promise;
    assert.strictEqual(result, val);
  }
});

test("original setInterval", async () => {
  const iter = timers.setInterval(1, "foo");
  const result = await iter.next();
  iter.return();
  assert.deepStrictEqual(result, { done: false, value: "foo" });
});

test("mocked setInterval", async (context) => {
  context.mock.timers.enable();
  const iter = timers.setInterval(1, "foo");
  const promise = iter.next();
  context.mock.timers.tick();
  const result = await promise;
  iter.return();
  assert.deepStrictEqual(result, { done: false, value: "foo" });
});

fails with

❯ node .\index.mjs
✔ original setTimeout (6.8319ms)
✖ mocked setTimeout (2.0594ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  2 !== false
  
      at TestContext.<anonymous> (file:///C:/Users/mfischer/src/test/node-mock-timer-setTimeout/index.mjs:18:12)
      at async Test.run (node:internal/test_runner/test:581:9)
      at async Test.processPendingSubtests (node:internal/test_runner/test:325:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: 2,
    expected: false,
    operator: 'strictEqual'
  }

(node:38604) ExperimentalWarning: The MockTimers API is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
✔ original setInterval (2.2276ms)
✖ mocked setInterval (1.0964ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
  + actual - expected
  
    {
      done: false,
  +   value: 'foo1'
  -   value: 'foo'
    }
      at TestContext.<anonymous> (file:///C:/Users/mfischer/src/test/node-mock-timer-setTimeout/index.mjs:36:10)
      at async Test.run (node:internal/test_runner/test:581:9)
      at async Test.processPendingSubtests (node:internal/test_runner/test:325:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: [Object],
    expected: [Object],
    operator: 'deepStrictEqual'
  }
[...]

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

Always

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

The mocked functions should return the value specified by the caller.

What do you see instead?

The mocked functions (sometimes) return something else.

Additional information

No response

[Onyx2406]

For setTimeoutPromisified:

To ensure that the specified value is returned, even if it's falsy, we can modify the line to check if result is 'undefined':
return resolve(result !== undefined ? result : id);

For setIntervalPromisified:

startTime += interval;
This increments the startTime by the interval, causing the value to change. To maintain the original value passed to setInterval, we can store it and return it instead of the incremented startTime.

So, first we can store the original value like:
const originalValue = startTime;

Then, in the callback, emit the originalValue instead of the incremented startTime:
emitter.emit('data', originalValue);

@mika-fischer

[mika-fischer]

For setTimeoutPromisified:

To ensure that the specified value is returned, even if it's falsy, we can modify the line to check if result is 'undefined': return resolve(result !== undefined ? result : id);

Disagree. It should always return the same value the the original setTimeout returns. So for undefined, it should also return undefined.

For setIntervalPromisified:

startTime += interval; This increments the startTime by the interval, causing the value to change. To maintain the original value passed to setInterval, we can store it and return it instead of the incremented startTime.

So, first we can store the original value like: const originalValue = startTime;

Then, in the callback, emit the originalValue instead of the incremented startTime: emitter.emit('data', originalValue);

That would work.

I don't know what the original intention behind returning these other values was. If they are not needed, I'd just remove them completely. If they are needed for some reason, they need to be communicated in some other way...

[benjamingr]

This seems like a pretty straightforward bug @nodejs/test_runner @ErickWendel where we should check if result is passed instead of using ||.

Your report and diagnosis seems correct @mika-fischer thank you for opening a good report.

Would you be interested in opening a PR to align setTimeout promisified with the non faked version?

[Onyx2406]

For setTimeoutPromisified:

To ensure that the specified value is returned, even if it's falsy, we can modify the line to check if result is 'undefined': return resolve(result !== undefined ? result : id);

For setIntervalPromisified:

startTime += interval; This increments the startTime by the interval, causing the value to change. To maintain the original value passed to setInterval, we can store it and return it instead of the incremented startTime.

So, first we can store the original value like: const originalValue = startTime;

Then, in the callback, emit the originalValue instead of the incremented startTime: emitter.emit('data', originalValue);

@mika-fischer

Should we use return resolve(result); and what do you think about setIntervalPromisified ? @benjamingr

[benjamingr]

That the fakes should all return the same types of results as the originals (even though I personally don't like these overloads) - the whole point of fake timers is that they match the originals.

[mika-fischer]

Would you be interested in opening a PR to align setTimeout promisified with the non faked version?

Yes, I can do a PR tomorrow.

[mika-fischer]

See #50331

Turns out setTimeout was already fixed in main

[mika-fischer]

For completeness, the setTimeout issue was already fixed here