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
794 stars 103 forks source link

New missing APIs #418

Open benjamingr opened 2 years ago

benjamingr commented 2 years ago

There are a few things missing in fake-timers we need to add for Node 17 and for new browser APIs namely:

None of these new APIs are particularly hard to mock but I haven't had a chance to look at them yet.

fatso83 commented 2 years ago

I think point two is done. At least I think I had something to do with it:

$ ag -A10 toPrimitive
src/fake-timers-src.js
599:                [Symbol.toPrimitive]: function () {
600-                    return timer.id;
601-                },
602-            };
603-            return res;
fatso83 commented 2 years ago

What is point one about? Like some example code (that we should mock) would help my feeble mind.

SimenB commented 2 years ago

import {setTimeout} from 'timers/promise'

SimenB commented 2 years ago

not sure if it's possible to mock? unless it's also available somewhere globally so we can replace it

fatso83 commented 2 years ago

That was new. Thanks.

benjamingr commented 2 years ago

Seems posible @SimenB

➜  node git:(master) node
Welcome to Node.js v16.10.0.
Type ".help" for more information.
> let orig = require('timers/promises').setTimeout;
undefined
> require('timers/promises').setTimeout = function (fn) { fn(); }
[Function (anonymous)]
> require('timers/promises').setTimeout(() => console.log('foo'), 10000); // logs immediately
foo
undefined

(Of course, it should return a promise and probably just reuse the util.promisify version - this is just to show it's mockable)

SimenB commented 2 years ago

ESM as well? But that's promising? 😃

thernstig commented 1 year ago

Not having timers/promises is becoming more and more of a nuance as Node.js 18 went to LTS October 2022, meaning more and more try to use it. Especially problematic in Jest where we now are resorted to using callback-based setTimeout() etc. when the other ones are nicer to read and use.

fatso83 commented 1 year ago

@thernstig Want to have a go? That's the best chance of finding something to scratch your itch. We accept PRs quite gladly 👍

thernstig commented 1 year ago

@fatso83 I try to help where I feel comfortable in the code but this is not such an area, otherwise I would already have pushed a PR.

What I wanted to highlight was that the new APIs have gone from experimental -> stable due to Node.js 18, possibly adding a new piece of information to this thread. I did not mean to sound pushy in a sense that I put any expectations on anyone to use their free time to fix it.

benjamingr commented 1 year ago

@itayperry wanna take a stab at it? I won't have time to take a look in the next ±2 months probably due to travel/life.

itayperry commented 1 year ago

Hi @benjamingr, It looks very interesting, and I'd like to try and help. I know you said it's supposed to be easy, but it seems hard.

It'd be great if you told me where to start 🚀 what's the first thing I should mock? Maybe we can even break it into very small tasks :) and then I could do a few small PRs. In that case, if I get stuck someone else could continue from where I stopped (last issue I had took me too long cause it was a bit tricky).

I'll give it a shot ⚡️

everett1992 commented 1 year ago

Also missing AbortSignal.timeout

fatso83 commented 1 year ago

@itayperry Would be great if you had a stab, and Ben already split these into smaller chunks. I would start with the bottom one in the list, then work my way up. AbortSignal.timeout is also a small and probably doable task that one could start with.

itayperry commented 1 year ago

@fatso83 sounds great 😊 I'll start with AbortSignal.timeout then :)

itayperry commented 1 year ago

Hi @fatso83 and @benjamingr, I've been reading about AbortSignal.timeout() static method and I'm not sure how to fake it.

The thing is, AbortSignal.timeout() is very different from other timers:

  1. It does not return an id (but an AbortSignal), for example:
let x = AbortSignal.timeout(10_000);

would first be: image

but after 10 seconds it would become: image

  1. It does not have a callback, just a value of time (number).
  2. It cannot (or doesn't-need-to) be cleared.

Plus, even if this timer was faked, how could we test it? It seems like we need a real fetch that uses a real URL to test it.

Do you have any idea how I can approach this? I'd love some advice :)

A small insignificant note: I tried using it with Google Chrome and it seems to respond with AbortError instead of TimeoutError, and there's an open bug about it: https://bugs.chromium.org/p/chromium/issues/detail?id=1431720&q=1431720&can=2

everett1992 commented 1 year ago

I'm not familiar with how sinon fake timers is implemented but when I need to mock AbortController.timeout() I'll use

timeout(n) {
  const controller = new AbortController()
  setTimeout(() => controller.abort(), n).unref()
  return controller.signal()
}

Then I can use fake timers and timeout will work as expected.

Testing should not require fetch - you can assert that signal .aborted is true after the timer expires

On Sat, May 20, 2023, 12:54 PM Itay @.***> wrote:

Hi @fatso83 https://github.com/fatso83 and @benjamingr https://github.com/benjamingr, I've been reading about AbortSignal.timeout() static method and I'm not sure how to fake it.

The thing is, AbortSignal.timeout() is very different from other timers:

  1. It does not return an id (but an AbortSignal), for example:

let x = AbortSignal.timeout(10_000);

would first be: [image: image] https://user-images.githubusercontent.com/37377389/239706213-1581b1b4-b432-4427-a8d2-fc01d2c142ca.png

but after 10 seconds it would become: [image: image] https://user-images.githubusercontent.com/37377389/239706757-97f8ab5b-6d45-4c91-a801-e409da2ea9a6.png

  1. It does not have a callback, just a value of time (number).
  2. It cannot (or doesn't-need-to) be cleared.

Plus, even if this timer was faked, how could we test it? It seems like we need a real fetch that uses a real URL to test it.

Do you have any idea how I can approach this? I'd love some advice :)

A small insignificant note: I tried using it with Google Chrome and seems to respond with AbortError instead of TimeoutError, and there's an open bug about it: https://bugs.chromium.org/p/chromium/issues/detail?id=1431720&q=1431720&can=2

— Reply to this email directly, view it on GitHub https://github.com/sinonjs/fake-timers/issues/418#issuecomment-1555982189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE6WM26LF4U77RA36MQZ3DXHEONRANCNFSM5NASE6KQ . You are receiving this because you commented.Message ID: @.***>

fatso83 commented 1 year ago

@everett1992 I am not actually 100% convinced that a DOM API like AbortController should be something we should target in fake-timers, but your solution seems like a possible approach, but it still leaves the issues @itayperry mentions on how this API should look like/work?

Say we approach it like you proposed: AbortSignal.timeout = timeout then we have the possibility to at least check if the timers/signal has been called by simply ticking and checking status flags:

const signal = AbortSignal.timeout(10000);
const fetchOp = fetch(url, {signal});
clock.tick(10000);
assert(signal.aborted);
clock.tickAsync(1);
try { await fetchOp }
catch (ex) {
  assert.instanceOf(AbortError)
}

Is this how you envision using it?

stale[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thernstig commented 6 months ago

Seems like a fair issue to unstale.