-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
chore: make tests compatible with Jest 24 #15779
chore: make tests compatible with Jest 24 #15779
Conversation
cda46a0
to
96fdb14
Compare
No significant bundle size changes to report. Generated by 🚫 dangerJS |
96fdb14
to
601f157
Compare
@@ -9,10 +9,6 @@ global.__PROFILE__ = NODE_ENV === 'development'; | |||
global.__UMD__ = false; | |||
|
|||
if (typeof window !== 'undefined') { | |||
global.requestAnimationFrame = function(callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsdom comes with raf
// - calling 'window.postMessage' should actually fire postmessage handlers | ||
global.requestAnimationFrame = function(cb) { | ||
return setTimeout(() => { | ||
cb(Date.now()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the raf from jsdom passes in the timestamp. is this safe to remove?
02f4bec
to
7d6f4a3
Compare
7d6f4a3
to
ae0dfe7
Compare
@threepointone I've rebased this PR. I'd love to see it land, then landing the upgrade to Jest 24 should be simpler as the diff will be cleaner |
@@ -21,4 +21,5 @@ module.exports = { | |||
collectCoverageFrom: ['packages/**/*.js'], | |||
timers: 'fake', | |||
snapshotSerializers: [require.resolve('jest-snapshot-serializer-raw')], | |||
testURL: 'about:blank', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that doing this breaks in JSDOM@>=11.12 with SecurityError: localStorage is not available for opaque origins
which is why the default has changed in Jest. Can try to do something for this when Jest 25 is released, which will force a newer version of JSDOM.
For now, I've manually made sure the entry in the lockfile is ~11.11.0, which does not have this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a short TODO here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ergh, expect it wasn't particularly short 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! I’ll merge it later today after I try it locally just to be doubly sure.
@@ -21,4 +21,5 @@ module.exports = { | |||
collectCoverageFrom: ['packages/**/*.js'], | |||
timers: 'fake', | |||
snapshotSerializers: [require.resolve('jest-snapshot-serializer-raw')], | |||
testURL: 'about:blank', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a short TODO here?
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
Outdated
Show resolved
Hide resolved
@@ -70,7 +70,7 @@ module.exports = { | |||
} | |||
: {} | |||
) | |||
).code; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this ensures you return source maps to Jest, for better errors and debugging.
81db1dd
to
081bd2e
Compare
@threepointone congrats on the 16.9 release 😀 I've rebased this after #16297 was merged |
@@ -651,7 +651,7 @@ describe('ReactDOMServerIntegration', () => { | |||
}); | |||
}); | |||
|
|||
describe('component hierarchies', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe
must be void
(in reality it must be synchronous) in Jest. async
here was always an error, but it throws with jest 24.
@@ -543,3 +543,4 @@ describe 'ReactCoffeeScriptClass', -> | |||
node = ReactDOM.findDOMNode(instance) | |||
expect(node).toBe container.firstChild | |||
undefined | |||
undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as the async
describe - the return value must be void
.
Since I was messing with the jest setup, I also sent #16332 which seems to have uncovered some erroneously passing tests |
25865db
to
74c40df
Compare
I merged the other one before this one, and this now has a conflict with yarn.lock. Sorry! 😅If you could rebase and make sure the lockfile is fine, I'll merge this in. Thank you! |
Co-Authored-By: Sunil Pai <threepointone@oculus.com>
74c40df
to
b7ce9a9
Compare
Rebased! 🙂 |
🎉 |
This is #15778 without the actual upgrade to Jest 24 (as that breaks CI). I'm hopeful this will pass so it can land, then the diff for Jest 24 is just config changes.
Note that this does upgrade jest to the latest 23 release, which is why the lockfile has so many changes