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

"Modern" fake-timer implementation doesn't work with PromiseJS. #10221

Closed
chrisbobbe opened this issue Jul 2, 2020 · 32 comments · Fixed by interledger/rafiki#387
Closed

"Modern" fake-timer implementation doesn't work with PromiseJS. #10221

chrisbobbe opened this issue Jul 2, 2020 · 32 comments · Fixed by interledger/rafiki#387

Comments

@chrisbobbe
Copy link

chrisbobbe commented Jul 2, 2020

Awesome work on #7776, thanks for that!!

🐛 Bug Report

I'm using Jest 26.1.0.

Testing async code with Promises, as described in this Jest doc, doesn't seem to work with jest.useFakeTimers('modern'); (from #7776; documented here) if you're using the 'promise' library from NPM.

For a while (starting in facebook/react-native@3ff3987), React Native has used that library as part of their Jest setup (which you get if you use preset: 'react-native'), so I expect this will affect most people using React Native (this is how I encountered it).

Otherwise, I'm not sure how often people use that 'promise' library, but it's an official solution given in a Jest troubleshooting doc, and pops up as a workaround for some issues in this repo (e.g. here).

This is arguably a regression (or would be when 'modern' becomes the default), as this behavior isn't observed with jest.useFakeTimers('legacy'); or jest.useRealTimers().

To Reproduce

E.g.,

global.Promise = require('promise');

jest.useFakeTimers('modern');

test('1 equals 1', async () => {
  await Promise.resolve();
  expect(1).toEqual(1);
});

Expected behavior

I expect the test to pass. Instead, five seconds go by and I get this failure:

  ✕ 1 equals 1 (5006 ms)

  ● 1 equals 1

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

      3 | jest.useFakeTimers('modern');
      4 | 
    > 5 | test('1 equals 1', async () => {
        | ^
      6 |   await Promise.resolve();
      7 |   expect(1).toEqual(1);
      8 | });

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Object.<anonymous> (fetchData.test.js:5:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        5.762 s, estimated 6 s
Ran all test suites.
error Command failed with exit code 1.

Link to repl or repo (highly encouraged)

In the repro linked below, I've closely imitated all the tests from the sections in the "Testing Asynchronous Code" doc that use Promises, to suggest that the failure happens in usual, well-documented ways of testing (provided you're using PromiseJS).

In its current state, you should observe these timeout errors on tests that use Promises (just run yarn and yarn test).

Try uncommenting global.Promise = require('promise'); at the top of fetchData.test.js; the tests should all pass.

Try doing something other than jest.useFakeTimers('modern');; the tests should all pass.

(Incidentally, note that accidentally calling jest.useFakeTimers multiple times with different arguments in the same file seems disruptive and might hamper your investigation. Also, be sure to comment out jest.runAllTimers() if you're using real timers.)

https://github.com/chrisbobbe/jest-async-modern-timers-repro

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 referenced this issue in facebook/react-native Jul 2, 2020
Summary:
We weren't exposing a mock for the batched bridge, which resulted in the WebSocket test failure but only in the open source copy of react-native.

public

Reviewed By: voideanvalue, vjeux

Differential Revision: D2782151

fb-gh-sync-id: e896097dd6060bc26963bc4c23db87b7277b3eba
@Hedi-s
Copy link

Hedi-s commented Jul 6, 2020

I have the same error with nock 12.0.3 and jest 26.1.0

The test case

test('nock', async () => {
    jest.useFakeTimers('modern');
    nock('http://modern').get("/").reply(200);
    await fetch(`http://modern`, { method: "GET" });
});

Failing :

 : Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:
  > 18 |         test('nock', async () => {
    |         ^
    19 |             jest.useFakeTimers('modern');
    20 |             nock('http://modern').get("/").reply(200);
    21 |             await fetch(`http://modern`, { method: "GET" });

It's working when I replace

  • 'modern' by 'legacy'
    or
  • fetch(...) by Promise.resolve() (nock is not actually used)

@SimenB
Copy link
Member

SimenB commented Jul 7, 2020

Hmm, I wonder if this is because we fake process.nextTick as well. As a test, can you modify your node_modules/@jest/fake-timers/build/modernFakeTimers.js (or wherever it's located) from the line const toFake = Object.keys(this._fakeTimers.timers); to const toFake = Object.keys(this._fakeTimers.timers).filter(name => name !== 'nextTick');. That should exclude it. This is the default behavior of @sinonjs/fake-timers, which I've changed in Jest's implementation.


As an aside, one should never replace global.Promise - that suggestion is outdated. E.g. when using async-await you will always get the native Promise regardless of the value of global.Promise.

@SimenB SimenB removed the Needs Repro label Jul 7, 2020
@Hedi-s
Copy link

Hedi-s commented Jul 7, 2020

modify your node_modules/@jest/fake-timers/build/modernFakeTimers.js (or wherever it's located) from the line const toFake = Object.keys(this._fakeTimers.timers); to const toFake = Object.keys(this._fakeTimers.timers).filter(name => name !== 'nextTick');.

The test with nock is working fine after doing it.
Since I tried 'modern' for _.debounce it's worth mentionning that a test I have with debounce is still working (It was not obvious to me).

@chrisbobbe
Copy link
Author

chrisbobbe commented Jul 7, 2020

As a test, can you modify your node_modules/@jest/fake-timers/build/modernFakeTimers.js (or wherever it's located) from the line const toFake = Object.keys(this._fakeTimers.timers); to const toFake = Object.keys(this._fakeTimers.timers).filter(name => name !== 'nextTick');.

Yep—when I do this, it works! Looks like sinonjs/fake-timers#323 is related.

As an aside, one should never replace global.Promise - that suggestion is outdated. E.g. when using async-await you will always get the native Promise regardless of the value of global.Promise.

Thanks for the heads-up! Yeah, a priori, I'm not really interested in replacing global.Promise. But I'm using React Native, with preset: 'react-native', so it's being done whether I like it or not (RN has been doing this since facebook/react-native@3ff3987). I suppose I could make a PR to react-native and fork it in the meantime, if it came to that.

@chrisbobbe
Copy link
Author

But I'm using React Native, with preset: 'react-native', so it's being done whether I like it or not

Opened facebook/react-native#29303 to find out more about React Native's motives here. 🔍

chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Jul 14, 2020
Unless otherwise noted, this commit will be unnecessary when
jacebook/jest#10221 is resolved.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 16, 2020
Trying this as a workaround for
jestjs/jest#10221; see
https://github.com/chrisbobbe/react-native/tree/pr-0.61.5-zulip for
the contents.

Currently breaks `tools/test android`.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 16, 2020
Trying this as a workaround for
jestjs/jest#10221; see
https://github.com/chrisbobbe/react-native/tree/pr-0.61.5-zulip for
the contents.

If we actually use this, we shouldn't use the fork associated with
my GitHub account, but rather zulip/react-native, as we discuss in
our doc:
https://github.com/zulip/zulip-mobile/blob/2c1caa2a2aa5e574fca3acfb4d4fda0ca2adde5c/docs/architecture/our-rn.md

Currently breaks `tools/test android`; looks like we have a doc that
may help us through that, at
https://github.com/zulip/zulip-mobile/blob/master/docs/howto/rn-from-git.md.
The "Android" section links to an upstream doc that looks
interesting for this.
@0xTomDaniel
Copy link

Hmm, I wonder if this is because we fake process.nextTick as well. As a test, can you modify your node_modules/@jest/fake-timers/build/modernFakeTimers.js (or wherever it's located) from the line const toFake = Object.keys(this._fakeTimers.timers); to const toFake = Object.keys(this._fakeTimers.timers).filter(name => name !== 'nextTick');. That should exclude it. This is the default behavior of @sinonjs/fake-timers, which I've changed in Jest's implementation.

As an aside, one should never replace global.Promise - that suggestion is outdated. E.g. when using async-await you will always get the native Promise regardless of the value of global.Promise.

After waisting a day troubleshooting why promises weren't working, this worked! Thank you.

I think Jest should resort back to the way Lolex was before. See sinonjs/fake-timers#114 for more context as to why Lolex didn't fake nextTick. Most people will not want the current behavior. Maybe Jest can provide an option to fake nextTick, but not by default.

@squareloop1
Copy link

Filtering out 'nextTick' works for native promises and should really be the default behaviour. Is there any workaround in the meantime for this without tinkering in node_modules to make this usable in CI/CD until a fix is out?

@0xTomDaniel
Copy link

0xTomDaniel commented Jul 30, 2020

Filtering out 'nextTick' works for native promises and should really be the default behaviour. Is there any workaround in the meantime for this without tinkering in node_modules to make this usable in CI/CD until a fix is out?

@squareloop1 I'd recommend using this nifty little tool. I just used it to patch Jest to the working fake-timers code. The patch even works seamlessly when updating a package. It's unfortunate, but I've also had to use it on several other packages that I'm using in my React Native app. A complete life saver.

@squareloop1
Copy link

squareloop1 commented Jul 30, 2020

@tomdaniel0x01 thanks, I think I will use fake timers from sinon for now, as they seemed to work. But I'd would rather get rid of another dependency and use it in jest. Its the only I am really missing in jest.

I have to add to this issue for clarity: It does not only affect non-native global.promise replacements, it also makes native promises in Node.JS with TypeScript stop working.

chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Aug 24, 2020
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Aug 25, 2020
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Aug 25, 2020
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Aug 25, 2020
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
chrisbobbe added a commit to chrisbobbe/react-native that referenced this issue Aug 25, 2020
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 25, 2020
Trying this as a workaround for
jestjs/jest#10221; see
https://github.com/chrisbobbe/react-native/tree/pr-0.61.5-zulip for
the contents.

If we actually use this, we shouldn't use the fork associated with
my GitHub account, but rather zulip/react-native, as we discuss in
our doc:
https://github.com/zulip/zulip-mobile/blob/2c1caa2a2aa5e574fca3acfb4d4fda0ca2adde5c/docs/architecture/our-rn.md

Currently breaks `tools/test android`; looks like we have a doc that
may help us through that, at
https://github.com/zulip/zulip-mobile/blob/master/docs/howto/rn-from-git.md.
The "Android" section links to an upstream doc that looks
interesting for this.
@jschoubben
Copy link

jschoubben commented Jul 13, 2021

I created a pull request to add the options object with an ignore property (notToFake) 100% inspired by the comment above. Any feedback is welcome and greatly appreciated.

The added code is kept deliberately simple and pragmatic, it can still be changed easily to reflect any input.
#11661

@danny-does-stuff
Copy link

@jschoubben Thank you! This would be so great if it got in

@deimantasa
Copy link

Is this moving along? As issue still persists.

@sagar-deshmukh09
Copy link

facing same issue. Have we fixed this issue or it's still in progress?

@szymonNowakAllegro
Copy link

szymonNowakAllegro commented Oct 13, 2021

What's the status? Will it be fixed?

@u-viktor
Copy link

+1

@abaklanov
Copy link

Possible workaround for the time being is using https://github.com/sinonjs/fake-timers, as it was mention before. But here's the example for those who stumbles across this thread:

import FakeTimers, { InstalledClock } from "@sinonjs/fake-timers";

const now = new Date("2021-11-25 11:00:00");

let clock: InstalledClock;
beforeAll(() => {
  clock = FakeTimers.install({
    now: now,
    shouldAdvanceTime: true,
    toFake: ["Date"],
  });
});

afterAll(() => {
  clock.uninstall();
});

Andarius added a commit to Andarius/react-native-reanimated that referenced this issue Jan 24, 2022
See this issue jestjs/jest#10221 for progress on support for the default 'modern' timer with react-native
piaskowyk pushed a commit to software-mansion/react-native-reanimated that referenced this issue Feb 2, 2022
Updating documentation for jest >= 27 since the 'modern' useFakeTimers does not work (yet) with react-native: jestjs/jest#10221

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [X] Updated documentation
- [ ] Ensured that CI passes
yiheyang pushed a commit to yiheyang/taro-reanimated that referenced this issue Feb 3, 2022
Updating documentation for jest >= 27 since the 'modern' useFakeTimers does not work (yet) with react-native: jestjs/jest#10221

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [X] Updated documentation
- [ ] Ensured that CI passes
@dagadbm
Copy link

dagadbm commented Mar 3, 2022

I tried using this but I keep getting jest warnings:

console.warn
A function to advance timers was called but the timers API is not mocked with fake timers. Call jest.useFakeTimers() in this test or enable fake timers globally by setting "timers": "fake" in the configuration file
Stack Trace:
at FakeTimers._checkFakeTimers (node_modules/@jest/fake-timers/build/modernFakeTimers.js:168:13)
at FakeTimers.advanceTimersByTime (node_modules/@jest/fake-timers/build/modernFakeTimers.js:100:14)
at node_modules/@testing-library/dom/dist/wait-for.js:90:18
at node_modules/@testing-library/dom/dist/wait-for.js:88:17
at unstable_advanceTimersWrapper (node_modules/@testing-library/dom/dist/config.js:26:40)
at node_modules/@testing-library/dom/dist/wait-for.js:87:15
at waitFor (node_modules/@testing-library/dom/dist/wait-for.js:39:10)

EDIT: Event after removing the waitFor it doesnt really work / advance time as it should and i get stuck on the fetch request.

@danny-does-stuff
Copy link

@dagadbm can you share more of your code? It's hard to see how your problem is related to this issue without more context

@dagadbm
Copy link

dagadbm commented Mar 4, 2022

https://github.com/dagadbm/load-monitoring-challenge

This component is the one I wanted to do an integration test on: https://github.com/dagadbm/load-monitoring-challenge/blob/master/front-end/src/features/metrics/Metrics.tsx#L24

As you can see we have a useEffect that dispatches a fetch call and also a dispatch for a pooling mechanism using setInterval every POOLING_RATE

This is the fetch code: https://github.com/dagadbm/load-monitoring-challenge/blob/master/front-end/src/features/metrics/metricsAPI.ts

I basically wanted to create a test that I mock the requests with MSW and then use jest fake timers (or sinon fake timers).

jest timers make the fetch to not even resolve.
Sinon fake timers are wildly inconsistent when I try to check the store state it is never what I expect it to be. (More details on the test below).

To have an idea of what I wanted to do you can check out this test I did where I manually do a "fake pooling" by dispatching actions manually on the store: https://github.com/dagadbm/load-monitoring-challenge/blob/master/front-end/src/features/metrics/Notification.test.tsx#L46

However, to really be integration, I would have loved for the setInterval to actually run so I dont have to manually dispatch all the actions to the store.

[...]

test('integration test - cpu above threshold', async () => {
  // i am using latest CRA version which has Jest version that doesn't even have the 'legacy' parameter
  jest.useFakeTimers(); 

  // this mocked request should be for this line of code:
  // https://github.com/dagadbm/load-monitoring-challenge/blob/master/front-end/src/features/metrics/Metrics.tsx#L26
  mockFetchCPUAverage(TIMESTAMP, DEFAULT_THRESHOLD);
  const { store } = render(<Metrics />);
 
  await waitFor( () => {
    // wait until we have the first result of the API call on the store
    expect(store.getState().metrics. cpuAverage.length).toBe(1);
    // default starting state
    expect(screen.getByText('No alerts in progress')).toBeInTheDocument();
  });

  // and from here on out I would expect, for each setInterval a different returned result from the API
  // similar to the notification test I showed before  

  // second request
  mockFetchCPUAverage(TIMESTAMP + 1, DEFAULT_THRESHOLD + 0.1);
  jest.advanceTimersBy(POOLING_RATE);
  await waitFor( () => {
    expect(store.getState().metrics. cpuAverage.length).toBe(2);
  });

  // third request
  mockFetchCPUAverage(TIMESTAMP + 1 + DEFAULT_ALERT_DELTA, DEFAULT_THRESHOLD + 0.1);
  jest.advanceTimersBy(POOLING_RATE);
  await waitFor( () => {
    // cpu is above load!
    expect(screen.getByText(`CPU above ${DEFAULT_THRESHOLD} load for more than ${DEFAULT_ALERT_DELTA} minutes.`)).toBeInTheDocument();
  });
});

I hope I wasnt too overwhelming. Let me know if I can be more explicit.

This is my test-utils file for the custom render function I copied from redux toolkit:

https://github.com/dagadbm/load-monitoring-challenge/blob/master/front-end/src/test-utils.tsx

@danny-does-stuff
Copy link

@dagadbm This issue is about the fact that jest fake timers do not work with promises, but in the example code you shared it looks like you are still using jest's fake timers (jest.useFakeTimers). The suggested solution from a comment above is to use sinon fake timers instead, because it does not mock the nextTick function by default.

However it seems that the issue you are seeing is more related to react-testing-library. I'm assuming that the error you were seeing (A function to advance timers was called but the timers API is not mocked with fake timers) has something to do with React Testing Library, because it looks like they try to do something with fake timers here. According to that Pull Request, it looks like you can just do something like this.

jest.useFakeTimers()
await waitFor(() => // next message to appear)
jest.useRealTimers()

And here's an example.

Code similar to yours with working tests
import React, { useRef, useEffect, useState } from 'react'
import { render, screen, act } from '@testing-library/react'
import FakeTimers from '@sinonjs/fake-timers'
import useInterval from 'someUseIntervalLib'

export const messages = ['message 1', 'message 2', 'message 3', 'the end']

const POOLING_RATE = 10_000
let fetchCount = 0
function fetchNewMessage() {
	console.log('fetching!')

	return new Promise(res => res(messages[fetchCount++ % messages.length]))
}

function Test() {
	console.log('rendering')
	const [message, setMessage] = useState(null)
	useInterval(() => {
		console.log('intervalling')

		fetchNewMessage().then(message => {
			setMessage(message)
		})
	}, POOLING_RATE)

	return (
		<div className="App">
			<h1>Your Message</h1>
			<h2>{message}</h2>
		</div>
	)
}

describe('Interval fetching', () => {
	it('fetches new stuff on an interval', async () => {
		jest.useFakeTimers()
		render(<Test />)
		expect(screen.getByRole('heading', { name: 'Your Message' })).toBeTruthy()

		for (let i = 0; i < messages.length; i++) {
			const message = messages[i]
			await waitFor(
				() => {
					expect(screen.getByRole('heading', { name: message })).toBeTruthy()
				},
				{ timeout: POOLING_RATE }
			)
		}

		jest.useRealTimers()
	})
})

aeddi pushed a commit to aeddi/react-native-reanimated that referenced this issue Mar 22, 2022
Updating documentation for jest >= 27 since the 'modern' useFakeTimers does not work (yet) with react-native: jestjs/jest#10221

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [X] Updated documentation
- [ ] Ensured that CI passes
@StanleySathler
Copy link

So jest.useFakeTimers() doesn't work within async functions, right?

Is this gonna be fixed any time soon so that we don't need to rely on @sinonjs/fake-timers?

@SimenB
Copy link
Member

SimenB commented Apr 6, 2022

Finally fixed in #12572, available in https://github.com/facebook/jest/releases/tag/v28.0.0-alpha.8.

@SimenB SimenB closed this as completed Apr 6, 2022
@github-actions
Copy link

github-actions bot commented May 7, 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 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.