fake-timers 7 doesn't seem to forward setTimeout args

[ronag]

setTimeout((foo) => {
  console.log(foo) // should log "hello"
}, 1, 'hello')

Refs: https://github.com/nodejs/undici/pull/675#issuecomment-812918698

[benjamingr]

@ronag hey - the args are just stored https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L785 and then just applied to the args https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L471

Any chance I could get a quick repro? The example in your code seems to work:

const fakeTimers = require('@sinonjs/fake-timers');

const clock = fakeTimers.install();

setTimeout((foo) => {
  console.log(foo) // this logs "hello"
}, 1, 'hello')

clock.tick(1);

[ronag]

We'll dig into it further. We had to pin in undici to v6.

[benjamingr]

Thanks, probably a good-first-issue for people to debug and figure out in unidici :)

[ronag]

@dnlup FYI

[dnlup]

@ronag hey - the args are just stored https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L785 and then just applied to the args https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L471

Any chance I could get a quick repro? The example in your code seems to work:

const fakeTimers = require('@sinonjs/fake-timers');

const clock = fakeTimers.install();

setTimeout((foo) => {
  console.log(foo) // this logs "hello"
}, 1, 'hello')

clock.tick(1);

Thanks, @benjamingr . Could it be something related to the interaction with nextTick or similar? This is the only instance that gives us problems in this test. Before calling setTimeout, this is indeed not undefined, but when it is referenced inside the callback, it is.

[fatso83]

I would set a breakpoint in the fake timers code to have a look at the arguments at the point they are applied. Debugging unknown client code is kind of costly, and this reference problems are usually a point in client code, so I am not all that inclined into digging into this right now. Still, if this is defined at the point of calling, I do not see why it should change underway, as the object this references should be treated just the same as any other argument, so this is a bit strange.

[ronag]

Just to be clear there are no problems in v6. The problem only happens in v7.

[fatso83]

Ooh! That was totally new to me.

[benjamingr]

Sounds like a fun bug. Debugging is relatively straightforward (run with --inspect-brk and step into the fake-timers code which is a single pretty readable file)

[benjamingr]

(and run jest in band (--runInBand) if using jest because jest does very "creative" stuff)

[dnlup]

Thanks for all the feedback, I'll try to inspect it and get back to you.

[dnlup]

Could it be the refresh logic?

• https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L364

It looks like the extra arguments are lost here, or am I missing something?

I have been inspecting and that looks like it could be the cause. We are refreshing timers on our code.

[fatso83]

@dnlup That seems like a good candidate! I agree with you. This seems like losing arguments.

[dnlup]

I can confirm that adding:

 setTimeout(timer.func, timer.delay, ...timer.args);

on that line resolves our issue.

[dnlup]

I can open a PR if you want.

[fatso83]

A tiny regression test would be great :)

[dnlup]

Should I modify src directly ot there is some transpiling that I have missed?

[benjamingr]

@dnlup src, PR welcome

[SimenB]

(and run jest in band (--runInBand) if using jest because jest does very "creative" stuff)

(not sure why Jest is brought up when the tests linked in this issue are using Tap, but since it was)

If you have examples where Jest behaves differently in band and not WRT fake timers I'd love to get a bug report with a reproduction. I don't remember having seen any issue about that before

[benjamingr]

@SimenB I saw the tests are using both Jest and Tap.

If you have examples where Jest behaves differently in band and not WRT fake timers I'd love to get a bug report with a reproduction. I don't remember having seen any issue about that before

This was just a general "in order to debug jest without waiting for the workers to spawn and quickly attaching the debugger to them" tip. I do that when I use Jest and I think (?) it's also the official recommendation at https://jestjs.io/docs/troubleshooting#tests-are-failing-and-you-dont-know-why

On the other hand - I do have a Jest performance issue I'd love to ask you about if you have 20 minutes of your time sometime. What would be a good way to reach out?

[SimenB]

Right, when debugging it's easier if you force not using workers (although running a single test will always run in band). I don't think using Node's worker_threads and child_process APIs necessarily should be classified as "creative" (which is what triggered my response), but that's neither here nor there (and most certainly off topic for this issue 😀).

What would be a good way to reach out?

Most often by opening an issue, but DM on twitter should be fine in this case 🙂

[benjamingr]

Thanks, messaged - this is me :) https://twitter.com/benjamingr_

[fatso83]

@benjamingr I updated your Twitter link. The underscore was left out (by Github/Markdown) when clicking, which made me wonder why you were speaking Spanish and just posting Samsung holiday offers 😄

See same link:

• https://twitter.com/benjamingr_
• https://twitter.com/benjamingr