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

Use Lolex to implement fake timers #5171

Closed
wants to merge 1 commit into from
Closed

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 25, 2017

Summary
Fixes #5165.

This PR switches out Jest's custom fake timers implementation with Lolex. This greatly simplifies things on our side (as we just need to proxy their API through the Jest object), while bringing many features:

Note that this might mean that previously working tests now will fail because more timers are faked. We can consider allowing jest.useFakeTimers to get passed an array matching Lolex's toFake if people want to specify what to mock and what not to. See https://github.com/sinonjs/lolex/blob/master/Readme.md#var-clock--lolexinstallconfig

It brings some behavioral (aka breaking) changes as well:

  • runAllPendingTimers in Jest currently do not trigger new timers scheduled while running to the last scheduled one. Lolex does (which matches what would happen in real code). Fixes calling runAllTimers after using Lodash's _.debounce results in an infinite recursion error #3465
  • runAllImmediates is removed. There is no way that real code could trigger an immediate without triggering other timers. Users can approximate this by doing jest.advanceTimersByTime(0), but that will also trigger timers.

Other breaking changes:

  • Callbacks scheduled using setImmediate or process.nextTick was scheduled using the real APIs in addition to the fake one, and Jest made sure that they only fired once (by keeping track). This has been removed - if you're faking time, then you're faking time
  • Globals that were scheduled for removal since Jest was open sourced in 2014 are removed (this landed in chore: remove long-deprecated timer globals #7285)
  • Jest added spies to the fake timers automatically. We no longer do that, people need to do e.g. jest.spyOn(global, 'setTimeout') themselves.
  • Timers scheduled to trigger after 0 ms will be coerced to 1, see Scheduling timers during tick with timeout of 0 forcing timeout of 1 sinonjs/fake-timers#166
  • runWithRealTimers removed - it was undocumented and unused at FB

Test plan
Green CI. No new tests are added as they'd be testing Lolex and not our own code. I've removed old tests that either didn't make sense or were testing features we've removed.

@thymikee
Copy link
Collaborator

I think we can start with not-purging FakeTimers and a temporary new environment and break it some time later.

@SimenB
Copy link
Member Author

SimenB commented Jan 5, 2018

I think we can start with not-purging FakeTimers and a temporary new environment and break it some time later.

You mean instead of using the fakeTimers we use currently, to have some separate channel?

@cpojer have you had a chance to look through the FB code base for usage of the different functions?

@cpojer
Copy link
Member

cpojer commented Jan 5, 2018

I checked, we don't use runWithRealTimers.

@SimenB
Copy link
Member Author

SimenB commented Jan 5, 2018

Cool, so sinonjs/fake-timers#146 and sinonjs/fake-timers#147 should be enough for us, then? That's awesome

@cpojer
Copy link
Member

cpojer commented Jan 5, 2018

We can try running this PR, once it's fully iterated, on the FB codebase to uncover more issues.

@Chudesnov
Copy link
Contributor

runAllImmediates()
is probably an anti-pattern as it breaks the abstraction.

But it is also the only way to advance code based on Promises in tests (scheduled with setImmediate, as mentioned in #5165 by @cpojer) without triggering other timers.

@cpojer
Copy link
Member

cpojer commented Feb 23, 2018

Are we planning to ship this or shall we close this PR for now?

@SimenB
Copy link
Member Author

SimenB commented Feb 23, 2018

There has been some movement in Lolex, I need to test if it's enough. Will look into it this weekend 🙂

@SimenB
Copy link
Member Author

SimenB commented Mar 4, 2018

I've rebased this and added the two potential changes in Lolex to help support our use case.

From my testing now, it's not enough, so I reported back on one of the PRs over there (sinonjs/fake-timers#157).

@SimenB
Copy link
Member Author

SimenB commented May 7, 2018

I've rebased now, and tested with sinonjs/fake-timers#164 merged. It's pretty close (I sent sinonjs/fake-timers#165 for a quick fix of one issue found) and works really well, but I'm having a few issues:

  • runOnlyPendingTimers (which I've implemented as calling Lolex' runToLast) runs all new timers added, not just the previously pending ones. Is that ok? I suppose this was known, but it's pretty breaking (I mentioned this in the OP as well, but it's been a while)
  • The throws before allowing infinite recursion test doesn't throw, but I think that's just a matter of fixing the test - the functionality works normally. (removed the test, see below)
  • runAllTicks still doesn't work... Calling Lolex' new runMicrotasks just makes the test hang forever, and I'm not sure why (Asked a question in their issue tracker, see link below)

@SimenB
Copy link
Member Author

SimenB commented May 7, 2018

Another key difference is that the timer function are no longer jest mocks when faked - people will have to do jest.spyOn themselves. Is that OK? Or should we spy on the functions by default?

Spying on them ourselves means we have to sniff out which ones exist, which it's sorta nice we don't have to now. E.g. if we had been using Lolex before, requestAnimationFrame would have been mocked without Jest changing at all.

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Figured out issue nr 3 above, it's the infinites recursion not making it bail. Might be same issue as number 2.

EDIT: Nope, not related, but asked a question in their issue tracker: sinonjs/fake-timers#147 (comment)

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Dug into number 2 above, and I think the thing there is that the recursion check is only for runAll, not tick. And Lolex has code forcing timers added during tick to have a timeout of 1, meaning when we advance by 50 ms, we get 51 timeouts. Not sure if it's by design, opening up an issue.

EDIT: sinonjs/fake-timers#166
EDIT2: That behavior is by spec, the behavior in Jest was wrong. So I just deleted the test 🎉

@thymikee
Copy link
Collaborator

thymikee commented May 8, 2018

Regarding this comment sinonjs/fake-timers#166 (comment), is it web only or Node acts in the same way?

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

My PRs to Lolex has been merged and released, so I updated this PR. Getting quite close now 🙂

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

@cpojer would it be possible to get an example of how you use fake timers to drive promises at FB? Would be great to add it as an integration test or something

@cpojer
Copy link
Member

cpojer commented May 8, 2018

cc @mjesun @rubennorte who can give more comments and/or run this diff on FB infra.

@SimenB SimenB force-pushed the lolex branch 2 times, most recently from 50c6b15 to 7b0d0e2 Compare May 13, 2018 17:08
@codecov-io
Copy link

codecov-io commented Oct 30, 2018

Codecov Report

Merging #5171 into master will increase coverage by 3.82%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5171      +/-   ##
==========================================
+ Coverage   62.19%   66.02%   +3.82%     
==========================================
  Files         266      240      -26     
  Lines       10701     9141    -1560     
  Branches     2603        6    -2597     
==========================================
- Hits         6656     6035     -621     
+ Misses       3459     3103     -356     
+ Partials      586        3     -583
Impacted Files Coverage Δ
packages/jest-environment-jsdom/src/index.js 36.84% <ø> (ø)
packages/jest-environment-node/src/index.js 52.17% <ø> (ø)
packages/jest-runtime/src/index.js 76.75% <75%> (ø)
packages/jest-util/src/fake_timers.js 77.14% <75%> (ø)
e2e/coverage-remapping/covered.ts 85.71% <0%> (-14.29%) ⬇️
...r/src/__performance_tests__/workers/worker_farm.js 0% <0%> (ø) ⬆️
...est-worker/src/__performance_tests__/workers/pi.js 0% <0%> (ø) ⬆️
packages/jest-circus/runner.js 0% <0%> (ø) ⬆️
...r/src/__performance_tests__/workers/jest_worker.js 0% <0%> (ø) ⬆️
packages/jest-haste-map/src/lib/WatchmanWatcher.js 0% <0%> (ø) ⬆️
... and 472 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a66bbb...61d7ac2. Read the comment docs.

@AlexLomm
Copy link

AlexLomm commented Nov 23, 2018

Hello, are there any updates / plans on this issue?

@thymikee
Copy link
Collaborator

@AlexLomm it's still being tested internally at FB. Let's be patient, there's no need to rush things ;)

@AlexLomm
Copy link

@thymikee No rush, just being curious :). Thanks for such a quick reply and keep up the amazing work! :)

@thymikee
Copy link
Collaborator

How do we stand with this one? Any feedback from internal testing? cc @rubennorte @rickhanlonii @aaronabramov

@SimenB SimenB removed this from the Jest 24 milestone Jan 11, 2019
@SimenB SimenB force-pushed the lolex branch 3 times, most recently from c202e0d to e13e60c Compare February 1, 2019 15:27
@SimenB
Copy link
Member Author

SimenB commented Feb 1, 2019

Non-breaking alternative: #7776

@SimenB
Copy link
Member Author

SimenB commented Mar 21, 2019

CI is very unhappy due to sinonjs/fake-timers#232

@Shahor
Copy link

Shahor commented Sep 4, 2019

Hi there 👋

Any chance of seeing this hitting release stage?

@SimenB
Copy link
Member Author

SimenB commented Sep 6, 2019

This is too breaking for FB as they rely on the broken semantics of the current implementation. So the current plan is to land a Lolex implementation in Jest 26 which will be the default if fake timers are used, and make the current implementation opt-in by saying jest.useFakeTimers('legacy'). That's in #7776, only flipped defaults.

I don't want to land Lolex as opt-in first to avoid juggling extra options

@github-actions
Copy link

This pull request 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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet