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

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

Merged
merged 4 commits into from
Feb 19, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 19, 2021

What:

Closes #898

Why:

callback might mock timers or timers might be mocked in another way.

How:

Check if we're in a jest-like environment via typeof jest !== 'undefined'.

Checklist:

  • [ ] Documentation added to the
    docs site
  • Tests
  • [ ] Typescript definitions updated
  • Ready to be merged

@eps1lon eps1lon added the bug Something isn't working label Feb 19, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 19, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8867abe:

Sandbox Source
react-testing-library-examples Configuration
react-testing-library demo Issue #898

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #900 (8867abe) into master (47e52d1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #900   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          941       942    +1     
  Branches       283       284    +1     
=========================================
+ Hits           941       942    +1     
Flag Coverage Δ
node-10.14.2 100.00% <100.00%> (?)
node-12 100.00% <100.00%> (?)
node-14 100.00% <100.00%> (?)
node-15 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/helpers.js 100.00% <100.00%> (ø)
src/wait-for.js 100.00% <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 a25149d...8867abe. Read the comment docs.

src/helpers.js Outdated
([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! 👍

The naming should indicate that they should only be called in a jest-like environment
@eps1lon eps1lon merged commit f7b5c33 into testing-library:master Feb 19, 2021
@eps1lon eps1lon deleted the fix/jest-global branch February 19, 2021 20:39
@github-actions
Copy link

🎉 This PR is included in version 7.29.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -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

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

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

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

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recently published 7.29.5 has ReferenceError: jest is not defined when mocha is used
4 participants