useFakeTimers in combination with async callbacks

[AlCalzone]

I'm currently trying to test an async method which sets a new timeout after it has completed executing. Using sinon's fake timers, this can be a minor headache. Basically I'm doing the following:

// async method to test:
async function foo() {
    console.log("before await");
    await someOtherAsyncMethod(); // <-- this has been stubbed out to return Promise.resolve(true)
    console.log("after await");
    //... some more synchronous work
    setTimeout(foo, 10000);
}
function bar() {
    setTimeout(foo, 10000);
}

// test method:
let clock = useFakeTimers();
let theStub = stub(...); // <-- this is the stub for someOtherAsyncMethod
bar();

for (let i = 1; i <= 5; i++) {
    console.log(i);
    clock.runAll();
    theStub.callCount.should.equal(i);
}

On the console you can see what happens:

1
before await
2
after await
=> test fails

I've been able to work around the issue by doing the following:

• creating a deferred promise (basically a promise with a method that allows to resolve it from outside) in every loop
• resolving that promise whenever a specific event gets emitted (hidden in the // some more work comment)
• awaiting that promise at the end of every loop:

for (let i = 1; i <= 5; i++) {
    console.log(i);
    // advance the timer and ensure the awaited method has been called
    clock.runAll();
    theStub.callCount.should.equal(i);
    // wait for the async callback to be executed until the end
    thePromise = createDeferredPromise();
    await thePromise;
    thePromise = null;
}

// somewhere else
on("event", () => thePromise && thePromise.resolve());

Now the log looks like this:

1
before await
after await
2
before await
after await
...

So, here's my suggestion:

Provide a method similar to runAll() (lets call it runAllAsync()) which does the following:

• collect the return values of all registered setTimeout callbacks
• returns a Promise that only resolves when all the return values have resolved

In case you're not familiar with async methods, they are basically functions that return a Promise but with some syntactic sugar. The proposed solution would work for async/await and promises alike.

This would make testing setTimeout with async methods a ton easier, as the above loop could simply be written like this:

for (let i = 1; i <= 5; i++) {
    console.log(i);
    await clock.runAllAsync();
    theStub.callCount.should.equal(i);
}

[dominykas]

Ran into a similar problem. My proposal would be to add a new option for useFakeTimers({ async: true }) and make a proper async variant of each function, i.e. you would await clock.tick(), await clock.runAll() whenever async is true, allowing all promises to resolve properly.

I might even come up with a PR this week if that's agreeable?

[mroderick]

I might even come up with a PR this week if that's agreeable?

That would be cool! https://github.com/sinonjs/lolex is where to point your PR.

[fatso83]

@dominykas Make sure you don't start creating a PR until you have read through sinonjs/fake-timers#105! There is an extended discussion there, and it has basically been stalled since July, which is unfortunate, as @kylefleming has done an extensive amount of work and there is lots of good discussion.

His PR seems to do what a lot of people want (judging from the comments).

I am not quite sure where or why it stalled, but I remember it all going a bit over my head details wise, and it's now a long time since it was discussed, so I am a bit reluctant to dig into it. Maybe it's time some new eyes had a look at it ... (hint, hint 👀).

[dominykas]

Thanks, will take a look - prior art always good :)

[dominykas]

That is a very well researched issue indeed - I'll need some time to take all of the ideas in, as well as the discussions in a related sinonjs/fake-timers#114. I think it got stalled waiting for feedback.

I think the implementation is roughly what I was going to do (i.e. an async tick returns a promise and actually makes the event loop take a break using setImmediate allowing microtasks to complete) , except with a different API (tickAsync() vs install({ async: true }).

I do have to dig deeper to understand the non-native promise library behaviors, esp. in the browsers. I'll try to get back to this ASAP - first thing is to test whether that PR solves the problem I have (I suspect it does).

[AlCalzone]

except with a different API (tickAsync() vs install({ async: true }).

I'm not entirely sure what's better - making the behavior "async" globally or forcing the developer to decide on every call. The latter one is probably on the safe side from sinonjs/lolex point of view.

[dominykas]

I'm not entirely sure what's better - making the behavior "async" globally or forcing the developer to decide on every call. The latter one is probably on the safe side from sinonjs/lolex point of view.

I'm not sure there's a "safe" call involved here. My proposal is to not make it global - it's to make the behavior consistent for each tick() for the single clock installation. Sure, having an explicit tickAsync() allows developers to mix-and-match as needed, but I do feel that mix/match approach is a foot-gun 👣🔫(I haven't thought about it in detail - just a hunch), so maybe it's not an option that sinon should provide. That said, we may as well do both?

My proposal mostly comes from what one of my colleagues said, and I tend to somewhat agree - clock ticks are async in JS, so they should probably behave that way when mocked too. I once thought that maybe sinon deliberately made the clock the way it is precisely because it wanted to make the code look more sync (sync code is a lot easier to grok under certain use cases - the test script being one of them). However with async/await that is no longer necessary.

Sorry, I still haven't had a chance to properly digest the implementation and what's missing in sinonjs/fake-timers#105, but I do wonder if there's a way to get a decision on which API is better in the meantime?

[mroderick]

I am in favour of expanding the API with *Async varieties of methods.

For a couple of reasons:

1. We should try our best to avoid breaking backwards compatibility. If we change the behaviour of existing methods, beyond fixing bugs, we risk breaking things for a lot of users. No one is yet depending on the *Async varieties.
2. As a programmer, I don't like that I have to look at a configuration somewhere else, in order to understand the meaning of a line of code that I am currently looking at ... or even worse, it might have a completely different behaviour between different files or codebases. Once I've understood clock.tickAsync() once, I don't have to look anywhere else to understand that line.

install({ async: someBooleanValue }).

// ... thousands of lines of tests

// how does this behave?
clock.tick();

[stale]

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.

[dominykas]

No no no! I promise I'll get back to this!

[dominykas]

Thanks, bot :)

[shokmaster]

No no no! I promise I'll get back to this!

Unkept promises? :P

[AlCalzone]

Rather rejected Promises 😜

[dominykas]

No @shokmaster, you have an incorrectly configured timeout value.

[alelavoie]

After struggling with a similar problem all morning, I found a dirty workaround that works when async calls are stubbed.

The solution is to simply call await in the test context to give a chance to promises to resolve. Technically, you would need to call await once for every level in your promise chain.

Here is a version of your code that should work:

// async method to test:
async function foo() {
    console.log("before await");
    await someOtherAsyncMethod(); // <-- this has been stubbed out to return Promise.resolve(true)
    console.log("after await");
    //... some more synchronous work
    setTimeout(foo, 10000);
}
function bar() {
    setTimeout(foo, 10000);
}

// test method:
let clock = useFakeTimers();
let theStub = stub(...); // <-- this is the stub for someOtherAsyncMethod
bar();
iterator();

async function iterator() {
    for (let i = 1; i <= 5; i++) {
        console.log(i);
        clock.runAll();
        await true; // You might need another line like this depending of what your stub returns.
        theStub.callCount.should.equal(i);
    }
}

Note that I'm not "happy" with that solution but it works. I'm also not 100% sure of what is happening under the cover - if anyone could give a better explanation, I would be grateful.

[kbkk]

I see clock.tickAsync() has been implemented - should this be closed?

[fatso83]

AFAIK this has been implemented in Lolex by @dominykas (thank you) and exposed in Sinon's API, ref all the *Async methods mentioned here: https://sinonjs.org/releases/latest/fake-timers/

• clock.tickAsync()
• clock.nextAsync()
• clock.runAllAsync()

So unless someone says otherwise, this is done :)