jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.17k stars 6.45k forks source link

useFakeTimers breaks with native promise implementation #7151

Closed ForbesLindesay closed 5 months ago

ForbesLindesay commented 6 years ago

🐛 Bug Report

Use the native Promise implementation breaks useFakeTimers

To Reproduce

Steps to reproduce the behavior:

jest.useFakeTimers();

test('timing', () => {
  Promise.resolve().then(() => console.log('promise'));
  setTimeout(() => console.log('timer'), 100);
  jest.runAllTimers();
  console.log('end');
});

Expected behavior

It should log:

This is because runAllTimers should trigger the async promise handler first, then the timeout delayed by 100ms, then return control.

Actual Behaviour

Link to repl or repo (highly encouraged)

https://repl.it/repls/PhysicalBriefCores

ForbesLindesay commented 6 years ago

The workaround I've found is to add:

global.Promise = require('promise');

to my setup.js

rickhanlonii commented 6 years ago

Hey @ForbesLindesay, thanks for filing and for the workaround 👌

Is this because native promises don't use timers under the hood like a library has to?

SimenB commented 6 years ago

Fake timers in Jest does not fake promises (yet: #6876), however - as you discovered - if you use a polyfill for Promise that uses either setImmediate or process.nextTick as its implementation, it'll work. I think this is working as intended (for now)?

rickhanlonii commented 6 years ago

@SimenB that's my thought as well

ForbesLindesay commented 6 years ago

I think you're right about why this doesn't work, but I don't think you're really right about this working as intended. Promises are a form of timer, and because in the past everyone was using polyfills, this used to work and only recently stopped working.

SimenB commented 6 years ago

The fact people use promises differently isn't Jest's responsibility - it was never a feature of Jest that you could run Promises using its fake timers. As you've noted, polyfilling it with something that uses timers as its implementation makes it work again

ForbesLindesay commented 5 years ago

The fact people use promises differently

i'm not clear on what you mean by this?

it was never a feature of Jest that you could run Promises using its fake timers

when fake timers were created, native Promises didn't exist. The intention was always that as new forms of "timer" were added to the language, fake timers would support them. For example, setImmediate is not a timer, but is supported. As you can see on https://repl.it/repls/PhysicalBriefCores

The goal of jest has, for a long time, included being easy to use and un-surprising. This is very surprising behaviour for fake timers to have.

KamalAman commented 5 years ago

I would like to expand on this issue since it gets amplified by uses of setTimeouts within the async code:

jest.useFakeTimers();

test('timing', async () => {
  const shouldResolve = Promise.resolve()
    .then(() => console.log('before-promise'))
    .then(() => new Promise(r => setTimeout(r, 20)))
    .then(() => console.log('after-promise'));

  setTimeout(() => console.log('timer'), 100);
  jest.runAllTimers();
  await shouldResolve;
  console.log('end');
});

Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.

Expected: before-promise -> after-promise -> timer -> end Actual: timer -> before-promise -> Hangs

This issue here is there is nothing to continuously advance the timers once you're within the promise world. shouldResolve will never resolve.

Switching to global.Promise = require('promise'); does seem like does the trick to resolve the issue for this particular use case. However in practice we have found the it does not work for all use-cases.

The best solution without replacing promises that i have come up for this is a utility function to continuouslyAdvanceTimers. However your results will still be out of order.

const _setTimeout = global.setTimeout;
function continuouslyAdvanceTimers() {
  let isCancelled = false;

  async function advance() {
    while (!isCancelled) {
      jest.runOnlyPendingTimers();
      await new Promise(r => _setTimeout(r, 1));
    }
  }

  advance();
  return () => {
    isCancelled = true;
  };
}

jest.useFakeTimers();

test('timing', async () => {
  const shouldResolve = Promise.resolve()
    .then(() => console.log('before-promise'))
    .then(() => new Promise(r => setTimeout(r, 20)))
    .then(() => console.log('after-promise'));

  setTimeout(() => console.log('timer'), 100);
  const cancelAdvance = continuouslyAdvanceTimers();
  await shouldResolve;
  cancelAdvance();
  console.log('end');
});

Expected: before-promise -> after-promise -> timer -> end Actual: timer -> before-promise -> after-promise -> end

lili21 commented 5 years ago

@KamalAman

test('timing', async () => {
  const shouldResolve = Promise.resolve()
    .then(() => console.log('before-promise'))
    .then(() => new Promise(r => setTimeout(r, 20)))
    .then(() => console.log('after-promise'));

  setTimeout(() => console.log('timer'), 100);
  await Promise.resolve()
  jest.runAllTimers() 
  await shouldResolve
  console.log('end');
});
ForbesLindesay commented 5 years ago

I don't think there's any point adding to this issue. The problem is clearly stated and defined. All this needs is for one of the jest maintainers to acknowledge that this is not working as intended, then someone can submit a patch to fix it.

It would be good if the "Needs more info" tag could be removed, since this quite clearly doesn't need more info.

Please refrain from "me-too" style comments.

mudlee commented 5 years ago

I love this issue, really. After one day sucking I found this and it works now. Miracle. Incredible that I have to do hacks like this to test an async functionality with a test framework that supports async.

UPDATE. Example fix:

while (!fixture.storageMock.update.mock.calls.length) {
  await Promise.resolve();
}
SimenB commented 5 years ago

Note that it is impossible, by JavaScript spec, for an async function to return anything other than native promises, so there's not anything we can do generically in Jest. This has to be solved in the engines themselves. You can do what #6876 documents (transpile everything), but that's not something Jest can decide to do for you.

See e.g. https://github.com/petkaantonov/bluebird/issues/1434 and https://github.com/sinonjs/lolex/issues/114. Your best bet is probably to follow the Lolex issue - both because Jest is going to move its fake timers implementation to be backed by Lolex, but also because Ben actually maintains Node, so any news on what would allow async functions to function (hah) correctly when faked is probably gonna be posted there.

If at some point there is a way to return custom Promise from async functions in Node, then we can look into adding APIs for it in Jest. Until then, we're unlikely to do anything

TomasBarry commented 4 years ago

For what it's worth, we have resorted to overwriting window.setTimeout when using a setTimeout in a promise chain:

// Place this in the test file/test block when you want to immediately invoke
// the callback to setTimeout
window.setTimeout = (fn: () => void, _timeout: number): void => fn()
tatethurston commented 4 years ago

Posting this work around in case it helps someone else:

await Promise.resolve().then(() => jest.advanceTimersByTime(milliseconds));

More context here: https://stackoverflow.com/questions/51126786/jest-fake-timers-with-promises/51132058#51132058

Broader example:

  function sleep(ms: number): Promise<void> {
    return new Promise((resolve) => {
      setTimeout(resolve, ms);
    });
  }

  export async function foo(fn: () => T, waitMs: number): Promise<T> {
     await sleep(waitMs);
     return fn();
  }
  it('calls fn after x milliseconds', async () => {
    jest.useFakeTimers();

    const fn = jest.fn(() => 3);
    const retVal = foo(fn, 1000);

    expect(fn).not.toBeCalled();
    await Promise.resolve().then(() => jest.advanceTimersByTime(1000));
    expect(fn).toHaveBeenCalledTimes(1);
    await expect(retVal).resolves.toBe(3);
  });
0xTomDaniel commented 4 years ago

For those looking for the solution to this problem when using jest.useFakeTimers("modern");

https://github.com/facebook/jest/issues/10221#issuecomment-654687396

AlvinLaiPro commented 3 years ago

Marked

cameron-martin commented 3 years ago

Since @sinon/fake-timers has async versions of all timer-advancing methods designed to also run microtasks (https://github.com/sinonjs/fake-timers/pull/237), could this functionality be exposed in jest to solve this issue?

bfaulk96 commented 2 years ago

@tatethurston I hate that this solution works, but it does. 😆 Thank you.

fernandopasik commented 2 years ago

I read that mocking nextTick could be the problem? I tried jest.useFakeTimers({ doNotFake: ['nextTick'] }); but that didn't solve this issue. I still don't get chained promises executed.

chlbri commented 2 years ago
 const cancelAdvance = continuouslyAdvanceTimers();

Thanks, Simple and efficient

dmitrybirin commented 1 year ago

Thanks for this thread and particular comment ❤️ I also hate that it works, but it does :) Also could be wrapped into helper function 🤔

async function withAllTimersRun(callback) {
  const cancelAdvance = continuouslyAdvanceTimers();
  const result = await callback();
  await cancelAdvance();
  return result;
}

So in tests it could be:

//...
await withAllTimersRun(() => someFunctionWithALotOfPromisesAndTimeouts());
//...
damdafayton commented 1 year ago

For what it's worth, we have resorted to overwriting window.setTimeout when using a setTimeout in a promise chain:

// Place this in the test file/test block when you want to immediately invoke
// the callback to setTimeout
window.setTimeout = (fn: () => void, _timeout: number): void => fn()

Thanks for this information. When I used it with @KamalAman 's solution it worked perfectly.

github-actions[bot] commented 6 months ago

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 30 days.

github-actions[bot] commented 5 months ago

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

github-actions[bot] commented 4 months ago

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.