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

runAllAsync and runToLastAsync do not run code in the microtask queue #483

Closed fatso83 closed 8 months ago

fatso83 commented 9 months ago

What did you expect to happen?

I expected that fake timers

    1) should advance past queued microtasks using runAllAsync
    2) should advance past queued microtasks using runToLastAsync

What actually happens

They do not.

How to reproduce

git clone https://github.com/fatso83/lolex ; 
cd lolex;
git checkout async-issues-run-all;
npm i; npx mocha test/issue-async.test.js

See https://github.com/fatso83/lolex/blob/async-issues-run-all/test/issue-async.test.js

benjamingr commented 9 months ago

Jest can do it because it transpiles (probably)

We can't do it because queueMicrotask isn't hookable from JS like nextTick, we can use an internal Node API but without connecting a debugger it'd be hard to write it in a stable way

fatso83 commented 8 months ago

We can't do it because queueMicrotask isn't hookable from JS like nextTick, we can use an internal Node API but without connecting a debugger it'd be hard to write it in a stable way

@benjamingr Could you elaborate on this a bit? I am not sure I understand at all. We do take control over queueMicrotask, delegating to clock.nextTick(), so after looking a bit at the code and throwing in some debugging I thought this was just about some bug in our implementation.

I see the clock.jobs array is not cleared. Rather it is doubled up before exiting:

after [
  { func: [Function (anonymous)], args: [], error: null },
  { func: [Function: afterWriteTick], args: [ [Object] ], error: null }
]

I am not quite sure why that second job is there, but you say there is something about the async versions we cannot control?

fatso83 commented 8 months ago

I made a go and I think I found the bug, unless I am missing something substantial. This little diff should be sufficient to get the runAllAsync test working (at least when I tried):

@@ -1578,6 +1578,8 @@ function withGlobal(_global) {
                     function doRun() {
                         originalSetTimeout(function () {
                             try {
+                                runJobs(clock);
+
                                 let numTimers;
                                 if (i < clock.loopLimit) {
                                     if (!clock.timers) {

I cannot claim that I understand all the minute details of fake-timers, but I did see that runJobs, which runs the micro tasks, were missing from the runAllAsync method.

I still see that the clock.jobs array is non-empty (containing that { func: [Function: afterWriteTick] ..} job after the test has completed), but functionally everything seems to be working. Could you explain what this is, @benjamingr ?

fatso83 commented 8 months ago

Fix in #485. I'll move the tests into the right places if I got it right 😄

SimenB commented 8 months ago

Closed by ^