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

Upgrade jest to v29 #21575

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion fixtures/dom/src/toWarnDev.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// copied from scripts/jest/matchers/toWarnDev.js
'use strict';

const jestDiff = require('jest-diff').default;
const {diff: jestDiff} = require('jest-diff');
const util = require('util');

function shouldIgnoreConsoleError(format, args) {
Expand Down
2 changes: 1 addition & 1 deletion fixtures/legacy-jsx-runtimes/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

const expect = global.expect;

const jestDiff = require('jest-diff').default;
const {diff: jestDiff} = require('jest-diff');
const util = require('util');

function shouldIgnoreConsoleError(format, args) {
Expand Down
11 changes: 7 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@babel/preset-react": "^7.10.4",
"@babel/traverse": "^7.11.0",
"abort-controller": "^3.0.0",
"abortcontroller-polyfill": "^1.7.3",
"art": "0.10.1",
"babel-eslint": "^10.0.3",
"babel-plugin-syntax-trailing-function-commas": "^6.5.0",
Expand All @@ -61,17 +62,19 @@
"eslint-plugin-no-function-declare-after-return": "^1.0.0",
"eslint-plugin-react": "^6.7.1",
"eslint-plugin-react-internal": "link:./scripts/eslint-rules",
"fbjs-scripts": "1.2.0",
"fbjs-scripts": "3.0.1",
"filesize": "^6.0.1",
"flow-bin": "^0.142.0",
"glob": "^7.1.6",
"glob-stream": "^6.1.0",
"google-closure-compiler": "^20200517.0.0",
"gzip-size": "^5.1.1",
"jasmine-check": "^1.0.0-rc.0",
"jest": "^26.6.3",
"jest-cli": "^26.6.3",
"jest-diff": "^26.6.2",
"jest": "^29.1.1",
"jest-cli": "^29.1.1",
"jest-diff": "^29.1.0",
"jest-environment-jsdom": "^29.1.1",
"jest-jasmine2": "^29.1.1",
"jest-snapshot-serializer-raw": "^1.1.0",
"minimatch": "^3.0.4",
"minimist": "^1.2.3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,8 @@ describe('createEventTarget', () => {
"right": 0,
"top": 0,
"width": 0,
"x": 0,
"y": 0,
}
`);
target.setBoundingClientRect({x: 10, y: 20, width: 100, height: 200});
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-mock-scheduler/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
"homepage": "https://reactjs.org/",
"peerDependencies": {
"jest": "^23.0.1 || ^24.0.0 || ^25.1.0",
"jest": "^23.0.1 || ^24.0.0 || ^25.1.0 || ^26.0.0 || ^27.0.1 || ^28.0.0 || ^29.0.0",
"scheduler": "^0.20.0"
},
"files": [
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
"homepage": "https://reactjs.org/",
"peerDependencies": {
"jest": "^23.0.1 || ^24.0.0 || ^25.1.0",
"jest": "^23.0.1 || ^24.0.0 || ^25.1.0 || ^26.0.0 || ^27.0.1 || ^28.0.0 || ^29.0.0",
"react": "^18.2.0",
"react-test-renderer": "^18.2.0"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-react/src/internalAct.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function act<T>(scope: () => Thenable<T> | T): Thenable<T> {
if (setTimeout._isMockFunction !== true) {
throw Error(
"This version of `act` requires Jest's timer mocks " +
'(i.e. jest.useFakeTimers).',
'(i.e. jest.useFakeTimers({legacyFakeTimers: true})).',
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/__tests__/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ env.beforeEach(() => {
} = require('react-devtools-shared/src/utils');

// Fake timers let us flush Bridge operations between setup and assertions.
jest.useFakeTimers();
jest.useFakeTimers({legacyFakeTimers: true});

// Use utils.js#withErrorsOrWarningsIgnored instead of directly mutating this array.
global._ignoredErrorOrWarningMessages = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ describe(dimmedColor, () => {
describe(ColorGenerator, () => {
describe(ColorGenerator.prototype.colorForID, () => {
it('should generate a color for an ID', () => {
expect(new ColorGenerator().colorForID('123')).toMatchInlineSnapshot(`
const color = new ColorGenerator().colorForID('123');
expect(color).toMatchInlineSnapshot(`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another example of jestjs/jest#6744

Object {
"a": 1,
"h": 190,
Expand Down
11 changes: 8 additions & 3 deletions packages/react-dom/src/__tests__/InvalidEventListeners-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,14 @@ describe('InvalidEventListeners', () => {

if (!__DEV__) {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.calls.argsFor(0)[0]).toMatch(
'Expected `onClick` listener to be a function, ' +
'instead got a value of `string` type.',
expect(console.error.calls.argsFor(0)[0]).toEqual(
expect.objectContaining({
detail: expect.objectContaining({
message:
'Expected `onClick` listener to be a function, instead got a value of `string` type.',
}),
type: 'unhandled exception',
}),
);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,17 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported because we're in a browser click event:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
// This one is jsdom-only. Real browser deduplicates it.
// (In DEV, we have a nested event due to guarded callback.)
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
]);
Expand All @@ -124,9 +124,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported because we're in a browser click event:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
]);
Expand Down Expand Up @@ -178,17 +178,17 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -260,17 +260,17 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -336,9 +336,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -406,9 +406,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -473,10 +473,10 @@ describe('ReactDOMConsoleErrorReporting', () => {
]);
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
// Reported by jsdom due to the guarded callback:
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -544,9 +544,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -630,18 +630,18 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported because we're in a browser click event:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
// Reported by jsdom due to the guarded callback:
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
// This one is jsdom-only. Real browser deduplicates it.
// (In DEV, we have a nested event due to guarded callback.)
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
]);
Expand All @@ -657,9 +657,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[
// Reported because we're in a browser click event:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
]);
Expand Down Expand Up @@ -705,10 +705,10 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
// Reported by jsdom due to the guarded callback:
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -776,9 +776,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -845,10 +845,10 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
// Reported by jsdom due to the guarded callback:
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -919,9 +919,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -988,10 +988,10 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect(console.error.calls.all().map(c => c.args)).toEqual([
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
// Reported by jsdom due to the guarded callback:
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down Expand Up @@ -1062,9 +1062,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
[expect.stringContaining('ReactDOM.render is no longer supported')],
[
// Reported by jsdom due to the guarded callback:
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
detail: expect.objectContaining({message: 'Boom'}),
type: 'unhandled exception',
}),
],
[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

'use strict';

describe('ReactDOMEventListener', () => {
describe('ReactDOMEventPropagation', () => {
Copy link
Collaborator Author

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.

let React;
let OuterReactDOM;
let InnerReactDOM;
Expand All @@ -16,12 +16,12 @@ describe('ReactDOMEventListener', () => {
beforeEach(() => {
window.TextEvent = function() {};
jest.resetModules();
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');
Comment on lines -19 to +24
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixes https://app.circleci.com/pipelines/github/facebook/react/21294/workflows/b23b2fbb-9d67-447c-b395-21591b60ebf1/jobs/411166.

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?

Copy link
Contributor

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

// 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`;
});
makes it resolve to differing modules somehow?

Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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 🙂

});
expect(OuterReactDOM).not.toBe(InnerReactDOM);
});
Expand Down
Loading