Upgrade lolex with async versions of all timer-executing calls

[kylefleming]

Summary

This pull request adds tickAsync(), nextAsync(), runAllAsync(), runToLastAsync() to parallel the existing synchronous calls. These new calls all return promises that resolve with what the synchronous call would have resolved with or reject with whatever error the synchronous call would have thrown.

This pull request obviously requires feedback from the core lolex team, so I'm happy to make any changes that might come up.

Goal

The goal of these functions is to provide those writing tests with tools to test code that uses promises without having to work around the fact that lolex executes all timers synchronously in a row.

Example

Consider these timers:

var result: = null;
setTimeout(async function () {
    result = await Promise.resolve(50);
}, 1000);
setTimeout(function () {
    assert(result === 50);
}, 2000);

Under normal circumstances, the promise callback would be executed immediately once the first timer is done executing, giving the second timer access to result. The order would be:

1) timeoutCallback1
2) promiseCallback
3) timeoutCallback2

However when using lolex to simulate time with:

clock.tick(2000);

causes the callbacks to be called in this order:

1) timeoutCallback1
2) timeoutCallback2
3) (synchronous return to test context and subsequent assertions)
4) promiseCallback

With these new async functions, you would simulate time with:

await clock.tickAsync(2000);

which would cause the callbacks to be called in this order:

1) timeoutCallback1
2) promiseCallback
3) timeoutCallback2
4) (asynchronous return to test context and subsequent assertions)

This
a) properly simulates the js engine's event loop when promises are involved, and
b) allows assertions to be checked after the promise returned by clock.*Async() has settled.

Implementation details:

These calls rely on the native setImmediate() and semi-rely on the behavior of the js engine to execute asynchronous callbacks in this order:

process.nextTick -> promise callbacks -> set* timers

Considerations

• I have tick() and tickAsync() proxying to a doTick() so that they can share code, since tick() was the largest of the current calls. If it's preferable to separate them, I'll be happy to do that.

• Right now you can surround tick() with a try/catch in order to catch any errors that the timers might throw. If one is thrown, tick() will rethrow the first thrown after all timer callbacks are executed. Users of tickAsync() might also think this would catch any errors that a promise might throw, especially if the error is unhandled by user code. However, this would not be the case (and cannot be since we let the system event loop execute the promise callbacks). I don't think there's any workaround to this, other than to just know it exists and to add it to the documentation. The user will have to test for errors directly using the promise that rejected with it.

• Above, I mentioned that it semi-relies on that order. When using native promises, if that order is used by the js engine, then all promise callbacks created from a timer and any promise callbacks that get queue from those callbacks (etc for all nested callbacks) will be fully executed before the next timer is called by lolex. This obviously relies on the js engine executing promise callbacks until none are available before calling any set* timer callbacks. If the user is using a promise polyfill that relies on setImmediate, however, then it will not possible to execute all callbacks, because lolex must push a callback onto the set* timer stack before giving up control to let the js engine process any pending promise callbacks. If those promise callbacks create more promise callbacks, then they will be after the lolex callback in the queue and thus not possible to execute reliably.

• I have if statements surrounding all async code. The if statement is checking for the presence of global.Promise. However, these methods don't have to rely on promises. They could just as easily accept a callback and return nothing. The theoretical callback-based async methods could then just be wrapped in a promisify call for users that want to use them as promises.
  I don't know how much this makes sense though, since these new async calls were created specifically to handle promises created in a timer, and they don't work as reliably for async code that uses the setImmediate family.
  If a user is testing code in an environment without native promises and they are using callbacks, they are testing code that either
  a) is not strictly async (does everything in the same event tick)
  b) uses process.nextTick or setImmediate/setTimeout or
  c) relies on native async calls like fs.open().
  As I mentioned, these methods can't reliably handle the setImmediate family and any code relying on engine-level async code wouldn't really be able to use lolex anyways.
  Therefore the only situation that a callback version makes sense would be in an environment where there aren't native promises and the user is testing asynchronous code that relies on process.nextTick. I don't know how common this is so I don't know if it's worth pursuing. I'm also of the opinion that new code should be using promises wherever possible, but I know this is a library so I'll let you as the core team decide if there should be a callback version.

(Side note: I've included my other pull request's commit with this one so there won't be a merge conflict later since they're modifying the same code. If that pull request ends up being rejected, I'll remove that code from this one)

[fatso83]

Wow, quite a bit of work. There is also the recent PR #102 that deals with asynchronicity. Would you have a chance to see if the two overlap?

[kylefleming]

Sure thing. The 2 PRs can both solve certain specific use-cases, but their main goals are different. This PR is aimed at being a companion to native promises. #102 (shouldAdvanceTime) is aimed at contexts where the user wants to change time, but not control it (though still have the option to control it). #85 explains it as "Time travel without freezing."

Use case where they could overlap

The shouldAdvanceTime PR allows promises to resolve by using a native setTimeout callback, but you'd have to be very careful to not allow multiple promises from different sources to resolve at the same time or to be dependent on an earlier one executing. To use the example from above, shouldAdvanceTime would not handle this situation:

var result: = null;
setTimeout(async function () {
    result = await Promise.resolve(50);
}, 1000);
setTimeout(function () {
    assert(result === 50);
}, 2000);
clock.tick(2000);

clock.tick() would still execute both setTimeout callbacks synchronously, at which point the assertion would fail, even though 20ms later the promise callback would be executed by shouldAdvanceTime's tick. If you put the assertion after result is set in the first callback, then it should work fine with shouldAdvanceTime, although each test would take an additional 20ms to complete.

Case for #102 shouldAdvanceTime

The main use-case that I can see for this is in contexts where calling an explicit clock.tick() would be hard (#85 gives an example). Setting shouldAdvanceTime allows time to flow semi-normally but still gives you the freedom to the mock the system clock and manually advance time. (semi-normally because everything is batched every 20ms or TIME_TICK)

Case for #105 clock.*Async()

The main use-case for this is any time both setTimeout (& family) and promises are involved. Using these methods assures the user that all callbacks will be called in the same order that they would have if it was being handled by the system event loop and allows you to asynchronously know when all callbacks are done executing and if any of the setTimeout callbacks threw an error.

Conclusion

I think they both have their place and solve different problems.

If #102 is being merged after #105, then I would recommend the clock.tick(TIME_TICK) @ lolex-src.js:404 be changed to clock.tickAsync(TIME_TICK). Since there is no need for the results to come back synchronously, the tickAsync() will allow callbacks to be executed in the right order that would otherwise not (minimum condition for failure: 2 setTimeouts and a promise callback)

[kylefleming]

In terms of naming, I think ideally lolex would use tick for the async version and tickSync for the synchronous version to mimic node and others. Since tick was already synchronous, I didn't want to presume any backwards-incompatible changes. If you are willing to have breaking changes, I would suggest deprecating tick for this version and adding tickAsync and tickSync. Then in the next major release, deprecating tickAsync, and making tick be the asynchronous version. This would leave tick and tickSync moving forward but would give people a migration path and the ability to convert to the async version immediately.

Also, I've thought of 1 more caveat/consideration that I'll add above. Right now you can surround tick() with a try/catch in order to catch any errors that the timers might throw. If one is thrown, tick() will rethrow the first thrown after all timer callbacks are executed. Users of tickAsync() might also think this would catch any errors that a promise might throw, especially if the error is unhandled by user code. However, this would not be the case (and cannot be since we let the system event loop execute the promise callbacks). I don't think there's any workaround to this, other than to just know it exists and to add it to the documentation. The user will have to test for errors directly using the promise that rejected with it.

[kylefleming]

Related: sinonjs/sinon#738, sinonjs/sinon#1120, sinonjs/sinon#1397
Possibly related: #69

[fatso83]

This is a really nice pull request with a good amount of work put into it, so big thanks for giving it to the greater community! 🎂

Other than my comment on polyfilling this also requires a bit of documentation so that people can get how to use this. I am not sure if the current README format suits long-form all that well (which I think some pieces might require), but you can give it a go?

Anyway, do a rebase before the next commit (or remove the commit from the previous PR) to make the diff smaller.

[fatso83]

This won't fly without a polyfill library for most browsers. setImmediate is not an Ecmascript standard and is only found in Node and Microsoft's javascript engines. See this for recommended workarounds.

[dominykas]

For the purposes of this PR... would falling back to setTimeout be good enough?

[fatso83]

@dominykas yes, I think so. if the performance implications bother people, they can supply a proper polyfill (there are lots of them) that uses fast versions (such as postMessage, ModificationObserver, etc)

[benjamingr]

I have a different approach (keeping it all synchronous) - what do you think about #114 @kylefleming ?

[benjamingr]

process.nextTick -> promise callbacks -> set* timers

I can guarantee we will keep this order in NodeJS - but as someone who worked on promise integration in platforms (a little bit) not all browsers do this - and not all promise libraries. For example the most popular promise libraries (Bluebird and Q) do not do this but instead schedule promise resolution as a macrotick and not a microtick.

[kylefleming]

@fatso83 I've been digging around trying to find the best solution that will work with the most platforms and user supplied Promise libraries. I have a few questions for you:

1. Which node/browser versions is lolex targeting?
2. I think if we do use a polyfill that it shouldn't touch the global environment or it risks contaminating the code it was meant to test. Do you agree?
3. Do you prefer a polyfill as an npm dependency or inlined into the code? The reason to inline it would be that we would cut out parts we didn't need. Browserify's timers polyfill doesn't work for us, so require("timers") isn't an option, unless we supply browserify with a different library. YuzuJS/setImmediate works but might need modifications (it modifies global and it includes code that isn't helpful to us)
4. Do you want me to include any 3rd party promise libraries as devDependencies in order to test how lolex interacts with them? This would be especially helpful with testing platforms without native promises otherwise we won't be able to test the new functions on them at all. If so we would be able to certify that lolex executes callbacks properly for those libraries. The main one I'm thinking of is Bluebird, but others could be included in this testing.
5. Would it make sense to add a configuration option to allow users to specify a custom promise library that lolex should return? (library side, not testing side)

[fatso83]

Thanks for the detailed list, @kylefleming. This is a biggie best left for the big boys. @mroderick and @mantoni, want to chime in? 😊 I'll try to address this to the best of my knowledge, but some bits are in murky waters.

1. I don't think this has been addressed, but I would assume we would be targetting the same set of browsers and engines as Sinon does. And since Sinon went pretty much "evergreen" in version 2, I guess it would make sense if lolex did too, as its main job is to support Sinon. And we are just on a new major version, so it would make sense to do a clean cut here and state this explicitly.
2. Absolutely agree
3. Inlined seems fine, although maintaining a custom version is never hot, it might be necessary.
4. This is basically integration testing. Custom test runs might be necessary, and I wouldn't mind them as devDependencies, as long as they are not included in the build.
5. I think this makes sense. Sinon got support for using a custom promise implementation in version 2 (when using the stub.resolves() feature), so it would be in line with that. On the other hand, a lot of code that is dealing with Promises, in a lib that is basically just about timers, might be a bit on the side.

My two cents.

[benjamingr]

I think if we do use a polyfill that it shouldn't touch the global environment or it risks contaminating the code it was meant to test. Do you agree?

Unless you explicitly install I agree.

The main one I'm thinking of is Bluebird, but others could be included in this testing.

If bluebird doesn't work with lolex (in terms of scheduling, it's supposed to) please let me know and I'll fix it on the bluebird side. We support swapping the scheduler anyway.

---

@kylefleming still, wouldn't it be better to make everything synchronous instead and not have promises defer to the nextTick queue in tests (so we can flush the queue before the end of the test and run assertions?).

Now that we have a process.nextTick implementation support (effectively meaning microtick semantics) we can probably do something like: process._setPromiseScheduler(function(fn) { enqueueJob({fn: fn, ...}); })

And run things one after the other synchronously since we have all the hooks.

Of course, this means we have to write process._setPromiseScheduler first - I want to know if there is interest first.

[kylefleming]

process.nextTick -> promise callbacks -> set* timers

I can guarantee we will keep this order in NodeJS

That's good to hear.

@kylefleming still, wouldn't it be better to make everything synchronous instead and not have promises defer to the nextTick queue in tests (so we can flush the queue before the end of the test and run assertions?).

For controlled setups, yes definitely.

I can imagine lolex having a clock.promiseScheduler such that you could call bluebird.setScheduler(clock.promiseScheduler)(with or without doing lolex.install first). This would also allow testing of any promise library that supports a custom scheduler. Since this would be meant to allow synchronous resolution, I would imagine that you would use the current synchronous .tick() methods.

Now that we have a process.nextTick implementation support (effectively meaning microtick semantics) we can probably do something like: process._setPromiseScheduler(function(fn) { enqueueJob({fn: fn, ...}); })

Would this be something that bluebird and other promise libraries would implement as a sort of unofficial standard where lolex would look for the existence of this method and call if it exists as a way of making lolex.install more automated?

Independent of implementation, I do agree that for synchronous resolution of user code with promises, the current situation isn't ideal (setting clock.setImmediate as the bluebird scheduler could still create situations where things resolve out of order.)

---

Native promises seem a bit trickier though. Setting a custom scheduler works for third party libraries that support it, but short of changing the native promise implementation in some way (either swapping for a library, or monkey patching the Promise prototype), I don't think there's any way lolex can resolve them synchronously.

You could make an argument that since lolex.install is mocking things related to time, it could also be within its purview to mock anything related to asynchronicity. I would leave up to the maintainers of lolex to decide though.

If we do go that route, I'm not actually sure you could do it without going all the way and swapping the whole promise implementation with another one (unless you know of a way?).

---

Given the discussion so far, giving lolex a promise microtask queue and hooking that into promise libraries' schedulers definitely makes the most sense for people using those libraries.

For native promises, I'm not sure if it's better to stub the global promise prototype or to let the event system handle it by abusing the system task queues (ala this PR). I think swapping out promise implementations is a fairly common thing to do, so it's not likely to be an issue. It would allow promises to be resolved synchronously with the rest of the timers. It does make the user's test code 1 more step removed from their production code since it would not only replace when the promise is resolved but would also replace the implementation of how it's resolved. On the other hand, the async method would keep it closer to the production code and would be able to handle promise implementations that don't allow custom schedulers, but would be slower (quite a bit slower on some systems) and require users testing code that uses promises (even internally) to write their test code using promises.

Either approach would be suitable for my current needs (the only thing I don't want to do is swap in another promise library across my whole application).

I'll hold off on finishing this PR though and let one of the core maintainers chime in first.

[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.

[mroderick]

I think this needs further consideration

[krzkaczor]

Cool stuff @kylefleming I can confirm that it works :) Would be cool to get it merged! @mroderick

[jacobEAdamson]

The implementation details may be arguable, but the overall behavior is exactly what I've been looking for! Excellent work @kylefleming !

[akhilkedia]

I can confirm that this patch almost perfectly works for me. If using await clock.tickAsync(), Things work great while using native promises!
Thank you for this, its really helpful!

One bug I seem to encounter -
Use await on a promise, and the said promise resolves after a setTimeout(), and clock is run with clock.runAllAsync() - In this case, the promises seem to resolved once runAllAsync() finishes running rather than when they should be. This issue is not present when using tickAsync()

[fatso83]

@benjamingr I have no strong feelings about this. If you can push this forward, and see that the work @kylefleming has put into it somehow ends up in the main branch, I think it would make a lot of people happy (judging from issues in the main Sinon repo).

[mroderick]

What do we need to do to get this merged?

[rakeshramakrishnan]

Would love this feature. What do we need to get this PR merged?

[benjamingr]

Honestly? As a "promises person" and maintainer of a bunch of promise libraries I don't think this is really the best path forward - we should consider working on #114 (comment) - we can also (for node) expose running microticks which would let us write the above entirely synchronously.

That said, if we want to - then all runAsync would require is to schedule for ourselves in the nextTick since order of microtasks is guaranteed for native promises (and people using Bluebird use setScheduler and don't have this problem to begin with).

The problem is sequencing things - for two reasons:

• First, we don't have a guarantee on the order of it at Node yet - it depends on fun things (like whether or not the timer is nested, or whether or not we're in the first turn of the event loop). We intend to do this eventually though.
• Second - the above PR (and I am very appreciative for the effort) picks an approach we should agree on first. It also needs more tests for nesting promises in timers and vice versa (there are a great cases). It would be great to steal the WPTs for this

---

That said, this approach is likely the most compatible one, it will work in browsers and Node and this issue is something people run into.

If you'd like I can commit to working on this after I'm back from Holiday (around mid-October) - I can rebase work on top of this commit (I want to make sure the changes are attributed to @kylefleming ) - if they want to keep working on it that's also cool.

[rakeshramakrishnan]

Should we look into making this completely non-blocking, i.e., not restricting it to the scope of micro-task queue and also include macro tasks?

Reason: on ticking the clock, if many events are generated (in-lieu of scheduling at intermediate times), the consumption of those events should not be blocked by the generation of subsequent events during clock.tick. Can we prevent the possibility of starvation of the event loop by microtasks?

[benjamingr]

Can we prevent the possibility of starvation of the event loop by microtasks?

Microtasks will always execute in front of the event loop in the future and mostly already do now.

[fatso83]

Merged as #237. Thanks @kylefleming! And thanks @dominykas for keeping the lights on :)