-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Upgrade jest to v29 #21575
Upgrade jest to v29 #21575
Conversation
Comparing: 9c3de25...8d0a41d Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
d7385e3
to
1f3a7ca
Compare
@@ -7,7 +7,7 @@ | |||
|
|||
'use strict'; | |||
|
|||
describe('ReactDOMEventListener', () => { | |||
describe('ReactDOMEventPropagation', () => { |
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.
Drive-by change. The suite name now matches the file name.
React = require('react'); | ||
jest.isolateModules(() => { | ||
OuterReactDOM = require('react-dom'); | ||
InnerReactDOM = require('react-dom'); | ||
}); | ||
jest.isolateModules(() => { | ||
InnerReactDOM = require('react-dom'); | ||
React = require('react'); | ||
OuterReactDOM = require('react-dom'); |
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.
Probably required due to jestjs/jest#10963. Though it's unclear why this test passed without this change using scripts/jest/config.source-www.js
but failed using scripts/jest/config.build.js
.
@SimenB Any idea if this is intended?
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.
Sounds odd that changing config impacts module loading. My only guess is
react/scripts/jest/config.build.js
Lines 38 to 46 in c1220eb
// Map packages to bundles | |
packages.forEach(name => { | |
// Root entry point | |
moduleNameMapper[`^${name}$`] = `<rootDir>/build/${NODE_MODULES_DIR}/${name}`; | |
// Named entry points | |
moduleNameMapper[ | |
`^${name}\/([^\/]+)$` | |
] = `<rootDir>/build/${NODE_MODULES_DIR}/${name}/$1`; | |
}); |
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 modules were all coming from the mapped paths.
To be clear: It makes sense that the used React
needs to be the same one that's used by OuterReactDOM
and therefore needs to be in the same isolatedModules
callback. What's odd is, that this isn't the case with scripts/jest/config.source-www.js
.
So ideally we'd have two isolated copies of react-dom
that somehow share the same react
copy. This no longer seems possible with Jest 27
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.
Seems you've worked around this, but a solution might be a new API which lets you selectively evict specific modules from the cache? jest.resetModule('react-dom')
or something?
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.
I haven't thought about this for a while but if I remember correctly, I came to the conclusiong that the old code should never have worked to begin with. Not sure if it's worth revisiting since this testing pattern is very specific.
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.
That sounds very reasonable 👍 It does sound like a weird edge case, but happy to revisit it if proves to actually guard against some regression 🙂
b358da4
to
0e0bb1d
Compare
45bac8e
to
0166dd3
Compare
Some CI shards are failing with out-of-memory errors. Going to take a while identifying the memory leaks. |
a86d1ea
to
e44ea79
Compare
Anything blocking this? |
I noticed an issue when running |
@gaearon jestjs/jest#11561 is blocking this update. Jest without |
Easiest to unblock is probably to float a patch (e.g. At least as long as babel config is available to babel's own way of looking it up. If not, straight to 29 might be needed |
This comment was marked as resolved.
This comment was marked as resolved.
ef13198
to
0e3a9e4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
TODO: investigate why we can't update snapshots
@@ -470,7 +484,6 @@ describe('ReactDOMFloat', () => { | |||
<head /> | |||
<body> | |||
foo | |||
<link rel="preload" as="style" href="foo" /> |
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.
According to the test, its actions should be run on the server. But as far as I can tell, there's only a client dispatcher (ReactDOMClientDispatcher
).
@gnoff Do you have a e2e test for this scenario so that we can verify where the test environment might not be setup properly?
Jest has become a lot stricter with mixing environments and these sort of tests break due to strict environment separation. IMO this is ok behavior from Jest since it makes it more obvious where particular code is running.
React Native and Metro have both upgraded to v29. Would be great if React could as well! 😀 |
Because it has merge conflicts and was not reviewed in its latest state |
Landed in #26088 |
Waiting for release of jestjs/jest#13328
Summary
abortSignal.reason = ...
hack in JSDOM environmentsTest Plan