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

feat: add new 'modern' implementation of Fake Timers #7776

Merged
merged 2 commits into from
May 3, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 1, 2019

Summary

This is an alternative to #5171 and #7300. Difference from them is that in this PR Lolex is added as an opt-in alternative to the current implementation of fake timers, instead of replacing it.

I've added jest.useFakeTimers('modern'); which will allow people to migrate test file by test file if they want the new system. People can also use useFakeTimers('legacy') if they want to keep using the old implementation.

The default if nothing is passed to jest.useFakeTimers (or just fake is passed in config) will be the legacy implementation in Jest 26, and we will swap the default in Jest 27. We have no plans to ever remove the legacy implementation.

Hopefully this means FB can migrate over over timer and we can remove Jest's implementation.

Fixes #3465
Fixes #5165
Closes #5171
Closes #7300

Test plan

Copied over the refactored unit test from #5171, and added an integration test.

@codecov-io
Copy link

codecov-io commented Feb 1, 2019

Codecov Report

Merging #7776 into master will decrease coverage by 0.10%.
The diff coverage is 16.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7776      +/-   ##
==========================================
- Coverage   64.06%   63.96%   -0.11%     
==========================================
  Files         293      293              
  Lines       12457    12478      +21     
  Branches     3067     3077      +10     
==========================================
  Hits         7981     7981              
- Misses       3833     3853      +20     
- Partials      643      644       +1     
Impacted Files Coverage Δ
...circus/src/legacy-code-todo-rewrite/jestAdapter.ts 0.00% <0.00%> (ø)
packages/jest-fake-timers/src/legacyFakeTimers.ts 90.35% <ø> (ø)
packages/jest-jasmine2/src/index.ts 0.00% <0.00%> (ø)
packages/jest-runtime/src/index.ts 56.33% <8.00%> (-1.69%) ⬇️
packages/jest-environment-jsdom/src/index.ts 38.00% <25.00%> (ø)
packages/jest-environment-node/src/index.ts 56.41% <25.00%> (ø)
packages/jest-fake-timers/src/modernFakeTimers.ts 69.76% <100.00%> (ø)

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 d7f3427...1342afc. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee commented Feb 1, 2019

Sounds good! Looking forward to @rubennorte review

@wmertens
Copy link
Contributor

Just wanted to add that unlike Jest, Lolex implements proper mocking for the Node timers, returning an object with .unref(), so looking forward to this!

@SimenB
Copy link
Member Author

SimenB commented Feb 18, 2019

Jest also returns ref and unref if you run your test in the node environment. Has done since Jest 22: #4622

@SimenB
Copy link
Member Author

SimenB commented Mar 21, 2019

Rebased this after the great TS migration.


I'm in favour of landing this in 24 and flipping the default in 25. While it sucks to ship multiple implementations of the same functionality (fake timers), I think using Lolex is such an improvement that it's worth it. Then people will have a whole major to migrate to the new implementation, and they can do it file by file if they want.

Not sure if deprecation warnings are worth it (especially as this is per file, so there'd be a lot of them)

@jeysal @mattphillips @thymikee @rubennorte @scotthovestadt thoughts?

@jeysal
Copy link
Contributor

jeysal commented Mar 21, 2019

I haven't followed this in detail - is it likely that switching the default will break a lot of users or did it only happen at FB because it's used in a weird/specific/rare way?
If it's the former I think deprecation warnings would be good so people are aware of upcoming migration work and have time to already migrate to Lolex. If it's the latter I'd say they are not needed.

Copy link

@quasicomputational quasicomputational left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning migration, I like the idea of flipping the default in 25. I also think it'd make sense to warn anyone explicitly opting in to Jest's fake timers (which they're presumably doing because something breaks with Lolex's; maybe they should be strongly encouraged to file bugs about that?). So then the plan goes something like this:

  • In 24. timers can be any of fake, real, jest or lolex; fake is equivalent to jest. No warnings are issued.
  • In 25, fake is equivalent to lolex. If jest is selected, there's a deprecation warning.
  • In 25 + n, jest is removed.

this._maxLoops = maxLoops || 100000;

this._fakingTime = false;
this._lolex = lolexWithGlobal(global);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be some way to tell Lolex what to fake? By default it will only fake methods it finds on the global passed in, but in the JSDOM environment queueMicrotask isn't there, which I'd like to have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it work for you to just polyfill queueMicrotask in a setup file, then Lolex would pick it up and fake it?


We discussed this (passing it what to mock) briefly in the fall. IIRC the argument against allow you to pass in args was that you shouldn't be able to opt out of certain methods - that would allow you to write tests in a way that would not reflect at all how it would run in the real world (beyond the fact you're faking time at all).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a dummy queueMicrotask in an environment would suffice, yeah. It'd feel a bit weird because you don't actually need to even attempt to polyfill it - just having that property on the object is enough! But if I let little things like that bother me, coding in JavaScript would drive me insane...

Being hard to use wrong is a good goal. And there's already an escape hatch in manually setting up Lolex in whatever crazy way you want it, so this isn't critical or anything.

@SimenB
Copy link
Member Author

SimenB commented Mar 21, 2019

Summary of how this is breaking in #5171. I'd wager to say that if your test fails after moving to Lolex, it's because it depends on bugs/broken behavior in Jest (barring the case where we now mock more than before, but that's ok, I think)

@SimenB
Copy link
Member Author

SimenB commented Mar 21, 2019

Thanks for chiming in @quasicomputational!

I really like your suggestion and I think we should go with that. I'd call the current implementation legacy instead of jest though - sounds a bit more scary so hopefully people want to avoid it.

Related, I'd also like to not call it lolex since the end goal is that it's an implementation detail. But calling it modern or something just seems like an unnecessary indirection

@quasicomputational
Copy link

Sounds good. Avoiding exposing the implementation detail in the name is an excellent principle. If I can bikeshed a bit, pre-25 and post-25, or v1 and v2, or something else explicitly versioned would be more future proof, just in case there's cause for another implementation switch down the line and suddenly modern isn't so modern.

@vovkasm
Copy link

vovkasm commented Apr 14, 2019

I'm highly interested in this. Does this feature has any ETA? Can I help?

@SimenB
Copy link
Member Author

SimenB commented Apr 14, 2019

I'll cry a thousand tears if I'm unable to land one of the 3 Lolex PRs for Jest 25, which is next up (no time estimate, though)

@vovkasm
Copy link

vovkasm commented May 12, 2019

Just to share with community (thought it was already mentioned in some of comments, but cant remember and without example), anyone can already use lolex as is. I use this setup for fully synchronous testing:

  1. Setup file for all tests (allow to use fake microtasks queue in Promise implementation), setup.js:
// Tune promises (we use Bluebird with direct scheduler so it can be mocked by lolex...)
global.Promise = require('bluebird')
global.Promise.setScheduler(function(fn) {
  setImmediate(fn) // or process.nextTick(fn)
})
  1. Distinct file to allow import it early testing/setupTestTimers.ts:
import lolex from 'lolex'
const clock = lolex.install({ toFake: Object.keys(lolex.timers) as any })
export { clock }
  1. Then in tests:
import 'jest'

import { clock } from 'testing/setupFakeTimers'

// code to test
function example() {
  return new Promise((resolve, reject) => {
    setTimeout(() => {
      resolve(true)
    }, 1000)
  })
}

beforeEach(() => {
  clock.setSystemTime(Date.parse('2018-04-03T14:44:00Z')) // Can set mocked time for ex
})

afterEach(() => {
  clock.reset()
})

it('example resolves to true after one second', () => {
  const cb = jest.fn()
  example().then(cb)
  clock.tick(500)
  expect(cb).not.toHaveBeenCalled()
  clock.tick(501)
  expect(cb).toHaveBeenCalledWith(true)
})

Notes.

  1. This will not (and cant actually in modern v8) mock native Promise and async/await, so you need polyfill custom (js-only) versions before your code will use them (I use bluebird in example and ban using of any async/await syntax in the project).
  2. This will not work with jest.fn().mockResolvedValue() function because this function will be evaluated before mock applied and will use native Promise.

@SimenB
Copy link
Member Author

SimenB commented Jul 21, 2019

Current plan is to land this as default in Jest 25, but allow people to do jest.useFakeTimers('legacy'); to opt-in to the old one. I'd rather now make the new implementation opt-in first, mostly so we don't have to juggle migration paths

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 13, 2021
We've entirely switched over to Jest's "modern" fake timers, which
landed in jestjs/jest#7776.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 13, 2021
We've entirely switched over to Jest's "modern" fake timers, which
landed in jestjs/jest#7776.
@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