Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
fatso83 opened this issue Oct 6, 2023 · 5 comments
Closed

Comments

@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2023

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
Copy link
Member

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
Copy link
Contributor Author

fatso83 commented Oct 19, 2023

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
Copy link
Contributor Author

fatso83 commented Oct 19, 2023

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
Copy link
Contributor Author

fatso83 commented Oct 19, 2023

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

fatso83 added a commit that referenced this issue Oct 20, 2023
* Add test from jestjs/jest#14549
* Fix for *Async time forwarders
@SimenB
Copy link
Member

SimenB commented Oct 20, 2023

Closed by ^

@SimenB SimenB closed this as completed Oct 20, 2023
fatso83 added a commit that referenced this issue Oct 20, 2023
Made the tests more to the point
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants