fix: Don't assume mocked timers imply jest fake timers #900
Conversation
|
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:
|
Codecov Report
@@ Coverage Diff @@
## master #900 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 941 942 +1
Branches 283 284 +1
=========================================
+ Hits 941 942 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/helpers.js
Outdated
| ([name, func]) => func !== globalObj[name], | ||
| ) | ||
| const usedJestFakeTimers = | ||
| typeof jest !== 'undefined' && |
There was a problem hiding this comment.
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,
}
}
...There was a problem hiding this comment.
At the cost of having multiple return points though. Not sure this would actually improve things.
There was a problem hiding this comment.
Might have a better idea.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@timdeschryver What do you think about a29bd91 (#900)? The branching makes more sense to me now.
The naming should indicate that they should only be called in a jest-like environment
and I don't care
|
🎉 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') { | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Possibly we could stick some non-enumerable symbol on the global object?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
What:
Closes #898
Why:
callbackmight mock timers or timers might be mocked in another way.How:
Check if we're in a
jest-like environment viatypeof jest !== 'undefined'.Checklist:
[ ]Documentation added to thedocs site
[ ]Typescript definitions updated