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 1 commit
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
6 changes: 3 additions & 3 deletions src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ function _runWithRealTimers(callback) {

const callbackReturnValue = callback()

const usedJestFakeTimers = Object.entries(timerAPI).some(
([name, func]) => func !== globalObj[name],
)
const usedJestFakeTimers =
typeof jest !== 'undefined' &&
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking if jest is defined everywhere it's used, could we return early if jest isn't defined?
I think this should also resolve #899

Something like this:

if (typeof jest === 'undefined') {
    const callbackReturnValue = callback()
    return {
      callbackReturnValue,
      usedJestFakeTimers: false,
    }
  }

 ...

Copy link
Member Author

Choose a reason for hiding this comment

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

At the cost of having multiple return points though. Not sure this would actually improve things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Might have a better idea.

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 like the early return but not in this function. I moved the check higher up the tree so that we now have dedicated functions for jest timers and framework agnostic timer functions. This also gets rid of the weird runWithRealTimers vs _runWithRealTimers where _runWithRealTimers is now runWithJestRealTimers

Copy link
Member Author

Choose a reason for hiding this comment

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

@timdeschryver What do you think about a29bd91 (#900)? The branching makes more sense to me now.

Copy link
Member

Choose a reason for hiding this comment

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

I like it! 👍

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

if (usedJestFakeTimers) {
jest.useFakeTimers(timerAPI.setTimeout?.clock ? 'modern' : 'legacy')
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