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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fake timers behave unexpectedly with Promise.race(). #10258

Closed
chrisbobbe opened this issue Jul 8, 2020 · 5 comments
Closed

Fake timers behave unexpectedly with Promise.race(). #10258

chrisbobbe opened this issue Jul 8, 2020 · 5 comments

Comments

@chrisbobbe
Copy link

chrisbobbe commented Jul 8, 2020

馃悰 Bug Report

With jest.useFakeTimers (either 'modern' or 'legacy'), a Promise.race between a short sleep Promise and a long sleep Promise...resolves to the long one's resolution, if the short one uses .then.

An identical Promise.race using real timers behaves as expected.

To Reproduce

For example, with modern fake timers (but also observed with the legacy fake timers):

describe('modern fake timers', () => {
  jest.useFakeTimers('modern');

  test('Promise.race without "then"', async () => {
    const shortPromise = new Promise(resolve => setTimeout(() => resolve('short'), 1000));
    const longPromise = new Promise(resolve => setTimeout(() => resolve('long'), 2000));
    
    const racedPromise = Promise.race([shortPromise, longPromise]);

    jest.runAllTimers();
    const result = await racedPromise;

    expect(result).toBe('short');
  });

  test('Promise.race with "then"', async () => {
    const shortPromise = new Promise(resolve => setTimeout(resolve, 1000)).then(() => 'short');
    const longPromise = new Promise(resolve => setTimeout(() => resolve('long'), 2000));
    
    const racedPromise = Promise.race([shortPromise, longPromise]);

    jest.runAllTimers();
    const result = await racedPromise;
    
    // Surprise! It's 'long'.
    expect(result).toBe('short');
  });
});

The second test, 'Promise.race with "then"', fails.

Expected behavior

I would expect the second test, 'Promise.race with "then"', to succeed; the shorter sleep should "win" the race, same as the version without "then". An identical test succeeds if you do jest.useRealTimers().

Link to repl or repo (highly encouraged)

Here, we have with-"then" and without-"then" tests for each of the following:

  • Modern fake timers (surprising behavior observed)
  • Legacy fake timers (surprising behavior observed)
  • Real timers (behaves as expected)

https://github.com/chrisbobbe/jest-modern-timers-race-then

envinfo

  System:
    OS: macOS 10.15.5
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
  Binaries:
    Node: 10.20.1 - ~/.nvm/versions/node/v10.20.1/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v10.20.1/bin/npm
  npmPackages:
    jest: ^26.1.0 => 26.1.0 
@chrisbobbe
Copy link
Author

chrisbobbe commented Jul 8, 2020

I expect a bit from MDN's doc on Promise.prototype.then will be relevant. It says (formatting lost):

Once a Promise is fulfilled or rejected, the respective handler function (onFulfilled or onRejected) will be called asynchronously (scheduled in the current thread loop). The behavior of the handler function follows a specific set of rules. [...]

I'm not sure if I'm reading this right, but it might suggest that it's quite impossible for any fake-timer implementation that uses native Promises to behave comparably to when you use real timers.

@MaxMotovilov
Copy link

MaxMotovilov commented Jan 22, 2021

Running into similar issues with multiple timers and promises. Wouldn't it help to have an asynchronous version of jest.runAllTimers() -- i.e. one that would setImmediate() between timer firings? This assumes promise implementation uses an equivalent of nextTick() to schedule the callback execution. Basically we need to be sure that all promise chains resolvable from a given timer would fully resolve themselves before the logically next (i.e. set to a later time) timer would fire.

By the looks of it, got to be a one-liner -- @sinonjs/faketimers runAllAsync() is already available but not used by Jest.

https://github.com/facebook/jest/blob/412954fd260f17976a450bd3b252a5d7e2488a64/packages/jest-fake-timers/src/modernFakeTimers.ts#L50

Can either add runAllTimersAsync() or a boolean async parameter to the existing call... For the time being, perhaps

jest._clock.runAllAsync()

could be used as a kludge.

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions
Copy link

github-actions bot commented May 5, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants