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

Proposed improvement for jest fake timers implementation #8891

Closed
jackgeek opened this issue Aug 30, 2019 · 8 comments
Closed

Proposed improvement for jest fake timers implementation #8891

jackgeek opened this issue Aug 30, 2019 · 8 comments

Comments

@jackgeek
Copy link

jackgeek commented Aug 30, 2019

🚀 Feature Proposal

Not sure if this should be Feature Proposal. Please let me know if I have used the wrong issue template. This is really an enhancement suggestion for the jest fake timers.

The enhancement suggestion is this: Make useFakeTimers and useRealTimers work as expected in every scenario.

Motivation

Presently the jest.useFakeTimers and jest.useRealTimers methods lead to surprising results.
This is due to how they are implemented. Here is my understanding of how they work:

  1. When the jest environment is created the methods that can be faked (setTimeout, etc) are captured by the jest-fake-timers package.
  2. When useFakeTimers is called the mockable methods (e.g. setTimeout) are replaced on the global object with a fake implementation
  3. When useRealTimers is called the mockable methods (e.g. setTimeout) are replaced with the captured real ones in Set rootDir for jest config #1

The problem is that when the useFakeTimers or useRealTimers is called modules imported by the test suite may have already captured the mockable methods and will hold on to those references so that calling useFakeTimers or useRealTimers puts the suite into a weird state where some of the loaded code is using real timers and some fake. This is never desirable.

I would like to propose that jest-fake-timers should instead always replace the mockable methods with a facade that will choose the correct implementation to defer to depending on the current desired state i.e. "real/fake".

This would avoid the aforementioned problem as everything would hold the references to the facade and the switch will not change the global object references.

Example of real developer experience when using the existing api

This is a cut-down real-world experience that my team and I had. Hopefully it illustrates why this change is important:

We set timers property in jest.config set to 'fake' as the majority of our code base requires fake timers.
However some tests need real timers. Here is how we attempted to solve this problem:

Attempt #1

import foo from './foo';
import waitForExpect from 'wait-for-expect';

test('foo does its thing', async () => {
  // Arrange
  jest.useRealTimers();

  // Act
  foo.doThing();

  // Assert
  await waitForExpect(() => {
    expect(foo.didItsThing()).toBeTrue();
  });
});

Result

Attempt failed because foo and waitForExpect already had fake setTimeout

Attempt #2

import foo from './foo';
import waitForExpect from 'wait-for-expect';

beforeEach(() => {
  jest.useRealTimers();
});

test('foo does its thing', async () => {
  // Arrange

  // Act
  foo.doThing();

  // Assert
  await waitForExpect(() => {
    expect(foo.didItsThing()).toBeTrue();
  });
});

Result

Attempt failed because foo and waitForExpect already had fake setTimeout

Attempt #3

jest.useRealTimers();
import foo from './foo';
import waitForExpect from 'wait-for-expect';

test('foo does its thing', async () => {
  // Arrange

  // Act
 foo.doThing();

  // Assert
  await waitForExpect(() => {
    expect(foo.didItsThing()).toBeTrue();
  });
});

Result

Attempt failed because foo and waitForExpect already had fake setTimeout
Wait! What!??
Yeah this is because jest hoists imports ES module imports are hoisted so foo module is executed before the jest.useFakeTimers() call...

Attempt #4

foo.test.js:

import '/test-utilities/useRealTimers';
import foo from './foo';
import waitForExpect from 'wait-for-expect';

test('foo does its thing', async () => {
  // Arrange

  // Act
 foo.doThing();

  // Assert
  await waitForExpect(() => {
    expect(foo.didItsThing()).toBeTrue();
  });
});

/test-utilities/useRealTimers.js:

jest.useRealTimers();

Result

Success! However now we realize that we cannot mix the use of real timers and fake timers in a single test-suite...

Sample facade implemention

Here is some pseudo code to illustrate how the facades could work:

let useFakeTimers = false;

jest.useFakeTimers = () => (useFakeTimers = true);
jest.useRealTimers = () => (useFakeTimers = false);

const realSetTimeout = global.setTimeout;
global.setTimeout = (...args) => (useFakeTimers ? fakeSetTimeout : realSetTimeout)(...args);

Example

Exactly as before except now you can safely use jest.useFakeTimers and jest.useRealTimers anywhere and the system will behave as expected.

Pitch

Why does this feature belong in the Jest core platform?
It is already in core.

@SimenB
Copy link
Member

SimenB commented Sep 2, 2019

Thanks for the detailed issue!

Here is my understanding of how they work:

That's correct.

The problem is that when the useFakeTimers or useRealTimers is called modules imported by the test suite may have already captured the mockable methods and will hold on to those references so that calling useFakeTimers or useRealTimers puts the suite into a weird state where some of the loaded code is using real timers and some fake. This is never desirable.

If modules go out of their way to capture these (doing const setTimeout = global.setTimeout I guess?) it's probably for a relatively good reason. If not, why not just do global.setTimeout()?

I'm not sure if this makes sense as part of core, it feels pretty invasive. I'm almost 100% sure people are gonna complain that they cannot capture real timers anymore. If you really want, you can use a custom test environment?


Yeah this is because jest hoists imports so foo module is executed before the jest.useFakeTimers() call...

That's the way ES modules work - imports are always evaluated first (it's Babel's module transform which hoists them, not Jest)

@jackgeek
Copy link
Author

jackgeek commented Sep 2, 2019

Hi @SimenB,

Thank you for taking the time to consider my request.

My first thought is that, in my tests, I do not want anything to be getting the real timer methods. However, I can see your point, if for example the wait-for-expect, or @testing-library/react wait command used fake timers that would not ideal... However you shouldn't mix async utilities with fake timers in the one test... But lets say hypothetically you had a module, which was not a production module being tested, rather a testing library for helping your test code, then I could see that you may want that to be unaffected by fake timers.

However I have to say that we do not have that requirement and we have a fairly large project (126K lines of code, 600 test suites and 4023 tests). Also the workaround for this would be quite simple: import the test utility to receive real timers in the jest-before-env-setup.js.

So I'm still of the opinion that perhaps this would be better solved by jest.

I am interested to hear more about your suggestion of creating a custom test environment. How would this help and how would you approach this?

Thanks again. 😊

Kind regards,
Jack

@melvinkcx
Copy link

I'm running into the same wall as @jackgeek did. My test suites are not working properly as setInterval in my Vue Component doesn't get stubbed with jest.useFakeTimers(). Still cracking my head around this.

@markdascher
Copy link

To give an actual example supporting @SimenB's concerns, here's something I've used to flush promises when fake timers are being used:

// How many times to flush before assuming an infinite loop.
const LOOP_LIMIT = 1000;

// Prevent useFakeTimers from overriding setTimeout below.
const setTimeout = window.setTimeout;

async function flushPromises() {
  for (let i = 0; i < LOOP_LIMIT; i++) {
    jest.runAllTimers();
    await new Promise((resolve) => {
      setTimeout(resolve, 0);
    });
    if (jest.getTimerCount() === 0) return;
  }
  throw new Error('infinite loooooop');
}

It's possible jest-before-env-setup.js would work for that? Not super familiar with that bit.

@jackgeek
Copy link
Author

Hi @markdascher,

With my proposal if your utility was imported in jest-before-env-setup.js it would capture the real setTimeout. It could then be used in any test-suite and work as expected because the setTimeout captured in its first import (in the jest-before-env-setup.js) would be the real setTimeout.

This feature request is quite old now and I have noticed that jest seems to behave slightly differently now: Regardless of whether you are have called useFakeTimers or useRealTimers or configured real or fake timers as defaults, if your test is async then real timers are always used.

This is unfortunate as we still live in an environment where it is not possible to fake native promises. I have encountered situations, recently using https://mswjs.io/, where I want to use fake timers and have an async test. This is because msw is working in a separate thread and waiting on a promise is unavoidable. This forces all tests using msw to be async and now means they all run with real timers. 🤔

@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 3, 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 3, 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

4 participants