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
797 stars 104 forks source link

Incompatible with promise-polyfill (a solution included) #363

Closed Finesse closed 3 years ago

Finesse commented 3 years ago

We understand you have a problem and are in a hurry, but please provide us with some info to make it much more likely for your issue to be understood, worked on and resolved quickly.

I make tests that run in different browsers (including IE 11) via Karma. IE doesn't support promises, so I include promise-polyfill. The following test case doesn't proceed in IE 11:

npm install @sinonjs/fake-timers@7.0.2 promise-polyfill@8.2.0 jasmine@3.6.4
include 'promise-polyfill/src/polyfill';
import * as fakeTimers from '@sinonjs/fake-timers';

it('mocks time', async () => {
  const clock = fakeTimers.install();

  try {
    const delayCall = (callback) => {
      new Promise((resolve) => setTimeout(resolve, 2000))
        .then(callback)
        .then(() => delayCall(callback));
    };

    const callback = jasmine.createSpy();
    delayCall(callback);
    await clock.tickAsync(10500); // Never resolves
    expect(callback.calls.count()).toBe(5);
  } finally {
    clock.uninstall();
  }
});

(Jasmine isn't required, any test runner can be used)

The promise polyfill uses the global setImmediate function, the fake-timers library mocks the setImmediate function and uses Promise inside tickAsync. What happens is that tickAsync calls resolve that calls the mocked setImmediate that doesn't resolve because fake-timers has locked it, i.e. fake-timers dead-locks itself.

If I disable mocking setImmediate using the toFake parameter, the callback will be called only 1 time instead of 5. It happens because the promise inside delayCall gets resolved after tickAsync decides to exit, because delayCall resolves several promises sequentially while tickAsync waits for new timers only during a single originalSetTimeout resolution. Since both Promise and originalSetTimeout wait using setImmediate, tickAsync exists before delayCall spawns a new timer. A solution is to make originalSetTimer wait longer than any number of promise-polyfill's _immediateFn calls. It can be achieved by implementing originalSetTimeout with setTimeout instead of setImmediate (because setImmediate always resolves before any setTimeout).

The ideal solution for all toFake values is to make promise-polyfill always use the original setImmediate and fake-timers to implement originalSetTimeout using setTimeout only. If you agree with this change and want me to make a MR, let me know. I'll make an issue in the promise-polyfill repository for this change.

At the moment I can fix the issue locally in my project by disabling setImmediate individually for fake-timers. It will both disable mocking setImmediate and will make originalSetTimeout not use setImmediate. The fix in my code example:

include 'promise-polyfill/src/polyfill';
import * as fakeTimers from '@sinonjs/fake-timers';

it('mocks time', async () => {
  // Remove setImmediate and then return back
  const originalProperty = Object.getOwnPropertyDescriptor(window, 'setImmediate');
  Object.defineProperty(window, 'setImmediate', { configurable: true, value: undefined });
  let fixedFakeTimers;
  try {
    fixedFakeTimers = fakeTimers.withGlobal(window);
  } finally {
    if (originalProperty) {
      Object.defineProperty(window, 'setImmediate', originalProperty);
    } else {
      delete window.setImmediate;
    }
  }

  const clock = fixedFakeTimers.install();

  try {
    const delayCall = (callback) => {
      new Promise((resolve) => setTimeout(resolve, 2000))
        .then(callback)
        .then(() => delayCall(callback));
    };

    const callback = jasmine.createSpy();
    delayCall(callback);
    await clock.tickAsync(10500); // Resolves quickly
    expect(callback.calls.count()).toBe(5);
  } finally {
    clock.uninstall();
  }
});
benjamingr commented 3 years ago

Opened a promise-polyfill PR: https://github.com/taylorhakes/promise-polyfill/pull/125

benjamingr commented 3 years ago

Looks like my PR was merged into promise-polyfill so I'll go ahead and close this :)

Finesse commented 3 years ago

@benjamingr Amending promise-polyfill isn't enough. This is also required:

... and fake-timers to implement originalSetTimeout using setTimeout only

This way fake-timers won't race with promise-polyfill