sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
793 stars 103 forks source link

NodeJS "timers" module is not mocked #466

Closed swenzel-arc closed 1 year ago

swenzel-arc commented 1 year ago

What did you expect to happen? NodeJS's timers module to be mocked upon FakeTimers.install().

What actually happens It's not mocked.

How to reproduce

let timers = require("timers");
let FakeTimers = require("@sinonjs/fake-timers");

let clock;

beforeEach(() => {
    clock = FakeTimers.install();
});

it("doesn't work", () => {
    let timerDone = false;
    timers.setTimeout(()=>{timerDone=true;}, 1000);
    clock.tick(1001);
    expect(timerDone).toBe(true);
});

afterEach(() => {
    clock.uninstall();
});

I was testing some code that is working with socket.io and was wondering why my sockets kept closing with ping time outs. Turns out engine.io is using timers.* instead of the corresponding global functions.
TBH I'm not quite sure if a fix belongs into this library or into jest's useFakeTimers function. However, the README here says it'd mock all "native" timers when FakeTimers.install is called without arguments, so it should maybe also include NodeJS's core module timers.

benjamingr commented 1 year ago

PR welcome :)

fatso83 commented 1 year ago

@benjamingr You are bit more into the Node internals than me, so I just vaguely remember this or something related (HTTP Headers in http using primordials?) coming up earlier. Are these timers (or the internal) easily stubbable somehow? Not totally sure where a volunteer would start. Jest would probably have an easier time doing this by just stubbing out calls to the timers lib, but we would somehow need to interface with the library directly. I would assume that trying to modify the exports directly would not work (this being ESM, using a immutable namespace and all).

benjamingr commented 1 year ago

The timers should be pretty easily stubbable unlike HTTP headers - just require('timers') and replace that object's setTimeout/setInterval properties

swenzel-arc commented 1 year ago

Thanks for the quick responses 🙏

PR welcome :)

I've had a quick glance at the fake-timers repo before opening this issue... but I wouldn't know where to put the code or how to make sure it's only done for NodeJS. Also I'm a senior Python developer, not so much JS/TS 😅

Jest would probably have an easier time doing this by just stubbing out calls to the timers lib

That's what I thought too and why I'm not sure whether a fix would belong here or there...

The timers should be pretty easily stubbable unlike HTTP headers - just require('timers') and replace that object's setTimeout/setInterval properties

Indeed they are and that's exactly my current workaround:

testUtils.ts

export function myUseFakeTimers() {
  jest.useFakeTimers();
  timers.setTimeout = setTimeout;
  timers.clearTimeout = clearTimeout;
  timers.setInterval = setInterval;
  timers.clearInterval = clearInterval;
  timers.setImmediate = setImmediate;
  timers.clearImmediate = clearImmediate;
}

export function myUseRealTimers() {
  jest.useRealTimers();
  timers.setTimeout = setTimeout;
  timers.clearTimeout = clearTimeout;
  timers.setInterval = setInterval;
  timers.clearInterval = clearInterval;
  timers.setImmediate = setImmediate;
  timers.clearImmediate = clearImmediate;
}
fatso83 commented 1 year ago

@swenzel-arc You already have code that works, so that is great. Now what about the interface ... We need to be able to selectively disable or enable the stubbing. I guess it makes sense to enable it as the default? And how should we deal with only stubbing selected timers? I think a reasonable assumption is that if you only want to stub out two specific timers from global, you would not want another selection for Node's timers module. If that assumption does not hold, we need a way of specifying it.

fakeTimers.install({ toFake: ['setTimeout', 'clearTimeout', 'nodeTimers']});

You would then basically add a section here to specifically deal with the Node timers lib.

I wouldn't know where to put the code or how to make sure it's only done for NodeJS This is the real question. If using CommonJS one can simply do checks to see if we are running Node or if the library can be loaded successfully, and only try stubbing if that is successful. Something like

let nodeTimersModule;
try { nodeTimersModule = require('timers') }
catch(err) { // not recent Node version  }

....
if (toFake.nodeTimers && nodeTimersModule) {
   installNodeTimers()
}

Conditional requires is not possible (AFAIK) when using ESM, as linking takes place before running the code, so I guess this would prevent us from easily going the ESM in the future? If we did, I think it would require clients to use something that captured calls to timers, which seems like a big nuisance. Unless you have some clever tricks to avoid this issue, @benjamingr?


Word of warning: instead of having an explicit section for dealing with Node timers, one could think (as I did) that we could just use the library's ability to target a specific object with its withGlobal(target) export. The whole installNodeTimers thing would then almost just be down to a single line: installNodeTimers(){ nodeTimersClock = withGlobal(nodeTimersModule); }. We could then just restore that as nodeTimersClock.restore() when restoring everything else. Unfortunately, you would then have to sync two sets of internal clocks (for instance when doing clock.tick()), which I do not think is complexity we want :-)

benjamingr commented 1 year ago

Conditional requires is not possible (AFAIK) when using ESM, as linking takes place before running the code, so I guess this would prevent us from easily going the ESM in the future? If we did, I think it would require clients to use something that captured calls to timers, which seems like a big nuisance. Unless you have some clever tricks to avoid this issue, @benjamingr?

A loader (not fun) or using top level await with dynamic import (easy and also works)

fatso83 commented 1 year ago

AFAIK, we cannot start using promise based code without changing the API. If we were to do dynamic imports (great idea, btw), we would probably have to change all the exports to resolve to promises:

export install(config:Config|null): Promise<Clock> 
export withGlobal(config:Config|null): Promise<Clock>
etc

Otherwise we would not know if the call to timers() was resolved or not.

```javascript let asyncFunction = async () => { async function main() { var value = await Promise.resolve("Hey there"); console.log("inside: " + value); return value; } var text = await main(); console.log("outside: " + text); }; console.log("pre"); asyncFunction(); console.log("post"); ``` will print ``` pre post inside: Hey there outside: Hey there ```

Either that, or expose a promise one could wait for: await clock.nodeTimers (or clock.asyncImportsResolved or similar).

swenzel-arc commented 1 year ago

It appears to be a bit harder than I thought... especially the uninstall part. Looks like clocks are supposed to hijack a single object and if there's more than one, then the mechanism for keeping track of hijacked methods has to be changed.
I guess I can do that, but seems to be a pretty invasive change.

swenzel-arc commented 1 year ago

Or actually, nevermind... I think I'll just handle the "timers" module separately. Similar to how I solved it in the workaround.

fatso83 commented 1 year ago

That's fine. We can still keep this issue around as we might want this still

benjamingr commented 1 year ago

Should this be closed or kept open for timers/promises?

fatso83 commented 1 year ago

Ah, good catch. I'll create a new one, referencing this.

swenzel-arc commented 1 year ago

Thanks guys 🙏

As it turns out, this doesn't solve my initial problem, though. Jest is using a custom global object but I made it so that the timers module isn't touched unless the patched object is the "default" global object. So upon jest.useFakeTimers the timers module stays the same.
Despite that, I still think it's not a good idea to have fake-timers change the timers module for every call to "install" as this might lead to unexpected behavior if multiple objects are being patched and reverted.

fatso83 commented 1 year ago

the referenced issue is # #469 btw

benjamingr commented 1 year ago

@SimenB is there anything we can do better in this regard for jest?

SimenB commented 1 year ago

Sniffing out require should work fine in Jest as that should work the same as Node, but that won't work for import declarations or expressions. I think that would need to hook into the module mocking directly. I haven't looked into it, tho

swenzel-arc commented 1 year ago

In case anyone want's to check it out: Jest creates the custom global object here which is then passed to their wrapper a few lines down here. The wrapper implementation is here.

Is there another way to access the "currently active global object"? Maybe we could compare the _global in withGlobal to something other than globalObject... which makes me wonder, with jest swapping out the global object, would the default install even patch the globals correctly?