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.

Sign up for GitHub

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

fix: Don't assume mocked timers imply jest fake timers #900

Merged
merged 4 commits into from
Feb 19, 2021
Merged

fix: Don't assume mocked timers imply jest fake timers #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3108e5b
fix: Don't assume micked timers imply jest fake timers
eps1lon Feb 19, 2021
a29bd91
Create dedicated jest timer functions
eps1lon Feb 19, 2021
bb44f29
No implicit return
eps1lon Feb 19, 2021
8867abe
I don't know how istanbul works
eps1lon Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ const TEXT_NODE = 3

// Currently this fn only supports jest timers, but it could support other test runners in the future.
function runWithRealTimers(callback) {
return _runWithRealTimers(callback).callbackReturnValue
// istanbul ignore else
if (typeof jest !== 'undefined') {
Copy link
Contributor

@SimenB SimenB Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

note that https://jestjs.io/docs/en/configuration#injectglobals-boolean is a thing, so if people enable that this check will give a false negative.

Not sure what the best solution is...

Copy link
Contributor

@SimenB SimenB Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Possibly we could stick some non-enumerable symbol on the global object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the pointer!

I'm honestly at the point where we should just create some wrapper library that requires passing some adapter to fake/real timers. That logic is just way too hairy for my liking and we have to consider quite a number of environments by now that we don't actually test with.

Using the exports field might be the best approach to stick with zero-config but I don't know if we could/should add an extra jest or mocha or browser entry.

PS: react has the same check so ideally we fix both instances so that we don't create even more pseudo-contexts (testing-library/react vs react in jest without globals).

Copy link
Contributor

@SimenB SimenB Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I came over this since I had to revert the @testing-library/dom update as it broke our tests using fake timers 😅

Not sure if Jest should put somewhere "I'm here, running code and stuff" or if libraries should take config. I like the former as a consumer but it requires you to keep detection (albeit a "blessed" one), and creating adaptors for everything doesn't sound scalable. And makes it potentially harder for other libraries to integrate cleanly... Dunno what's best.

exports could work, but as of now Jest doesn't support it and it's unlikely to land in time for Jest 27 (jestjs/jest#9771 is waiting for upstream resolve support)

Copy link
Member Author

Choose a reason for hiding this comment

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

I came over this since I had to revert the @testing-library/dom update as it broke our tests using fake timers sweat_smile

Which version did you have to rollback?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any way jest exposes for us to detect whether we're running in a jest environment? I think that this feature is very useful and considering the vast majority of users are using jest it seems appropriate to include even as a "blessed" solution. I think it would make sense to support other popular runners as well assuming it doesn't complicate the codebase too much.

Is there an environment variable or something we can use to reliably detect whether we're running in a jest environment?

Copy link
Contributor

@SimenB SimenB Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I came over this since I had to revert the @testing-library/dom update as it broke our tests using fake timers sweat_smile

Which version did you have to rollback?

We were (and are back to) using 7.29.4. Got 7.29.6 when updating, did not try .5. Note that we're using so-called "modern" timers which are back by sinon (former lolex). Opt-in in Jest 26 but default from v27 which will hopefully be released this month.

Is there an environment variable or something we can use to reliably detect whether we're running in a jest environment?

Not at the moment, that's what I was suggesting adding with the Symbol. I guess we could put something on process.env, tho, that sounds like a good idea. That might make sense regardless even though it requires polyfilling process.env for browser usage to use it. Jest won't be the runner in the browser though (at least not any time soon), so I guess that's not really a concern

Copy link
Contributor

@SimenB SimenB Mar 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We have JEST_WORKER_ID, maybe you can use that?

https://jestjs.io/docs/environment-variables#jest_worker_id

It will be set for everybody using jest-worker, but I think that's fine?

return runWithJestRealTimers(callback).callbackReturnValue
}

// istanbul ignore next
return callback()
}

function _runWithRealTimers(callback) {
function runWithJestRealTimers(callback) {
const timerAPI = {
clearImmediate,
clearInterval,
Expand All @@ -18,29 +24,33 @@ function _runWithRealTimers(callback) {
setTimeout,
}

// istanbul ignore else
if (typeof jest !== 'undefined') {
jest.useRealTimers()
}
jest.useRealTimers()

const callbackReturnValue = callback()

const usedJestFakeTimers = Object.entries(timerAPI).some(
const usedFakeTimers = Object.entries(timerAPI).some(
([name, func]) => func !== globalObj[name],
)

if (usedJestFakeTimers) {
if (usedFakeTimers) {
jest.useFakeTimers(timerAPI.setTimeout?.clock ? 'modern' : 'legacy')
}

return {
callbackReturnValue,
usedJestFakeTimers,
usedFakeTimers,
}
}

const jestFakeTimersAreEnabled = () =>
Boolean(_runWithRealTimers(() => {}).usedJestFakeTimers)
function jestFakeTimersAreEnabled() {
// istanbul ignore else
if (typeof jest !== 'undefined') {
return runWithJestRealTimers(() => {}).usedFakeTimers
}

// istanbul ignore next
return false
}

// we only run our tests in node, and setImmediate is supported in node.
// istanbul ignore next
Expand Down
6 changes: 3 additions & 3 deletions src/wait-for.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ function waitFor(

const overallTimeoutTimer = setTimeout(handleTimeout, timeout)

const usingFakeTimers = jestFakeTimersAreEnabled()
if (usingFakeTimers) {
const usingJestFakeTimers = jestFakeTimersAreEnabled()
if (usingJestFakeTimers) {
checkCallback()
// this is a dangerous rule to disable because it could lead to an
// infinite loop. However, eslint isn't smart enough to know that we're
Expand Down Expand Up @@ -107,7 +107,7 @@ function waitFor(
finished = true
clearTimeout(overallTimeoutTimer)

if (!usingFakeTimers) {
if (!usingJestFakeTimers) {
clearInterval(intervalId)
observer.disconnect()
}
Expand Down