Skip to content

Upgrade jest #4401

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
May 8, 2023
Merged

Upgrade jest #4401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
25c5edd
Upgrade testing tools
snowystinger Apr 18, 2023
b8aa4ba
fix alphabetical order
snowystinger Apr 18, 2023
35a31af
remove all legacy timers
snowystinger Apr 18, 2023
e972690
fix lint
snowystinger Apr 18, 2023
cbf086f
fix lint for real
snowystinger Apr 18, 2023
030e4a9
fix test
snowystinger Apr 18, 2023
ac7dec8
fix snapshot tests
snowystinger Apr 18, 2023
543e19b
Fix 16 and 17 tests
snowystinger Apr 18, 2023
587ae76
fix some alphabet
snowystinger Apr 19, 2023
599f183
Merge branch 'main' into upgrade-jest
snowystinger Apr 25, 2023
4d6d30b
Merge branch 'main' into upgrade-jest
snowystinger May 5, 2023
bf4b4db
Merge branch 'main' into upgrade-jest
snowystinger May 8, 2023
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
10 changes: 9 additions & 1 deletion lib/jestResolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,15 @@ try {
}

module.exports = (request, options) => {
let resolved = options.defaultResolver(request, options);
let resolved = options.defaultResolver(request, {
...options,
// https://github.com/microsoft/accessibility-insights-web/pull/5421#issuecomment-1109168149
packageFilter: pkg => {
delete pkg['exports'];
delete pkg['module'];
return pkg;
}
});
if (/packages\/.*\/.*\/intl\/.*\.json$/.test(resolved)) {
let sourceText = fs.readFileSync(resolved, 'utf8');
let json = JSON.parse(sourceText);
Expand Down
34 changes: 16 additions & 18 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
},
"scripts": {
"check-types": "tsc && tsc-strict",
"install-16": "yarn add -W react@^16.8.0 react-dom@^16.8.0 @testing-library/react@^12 @testing-library/react-hooks@^8",
"install-17": "yarn add -W react@^17 react-dom@^17 @testing-library/react@^12 @testing-library/react-hooks@^8",
"install-16": "yarn add -W react@^16.8.0 react-dom@^16.8.0 @testing-library/react@^12 @testing-library/react-hooks@^8 @testing-library/dom@^8",
"install-17": "yarn add -W react@^17 react-dom@^17 @testing-library/react@^12 @testing-library/react-hooks@^8 @testing-library/dom@^8",
"start": "cross-env NODE_ENV=storybook start-storybook -p 9003 --ci -c '.storybook'",
"build:storybook": "build-storybook -c .storybook -o dist/$(git rev-parse HEAD)/storybook",
"build:storybook-16": "build-storybook -c .storybook -o dist/$(git rev-parse HEAD)/storybook-16",
Expand Down Expand Up @@ -83,9 +83,9 @@
"@storybook/components": "^6.5.12",
"@storybook/react": "^6.5.12",
"@storybook/testing-react": "^1.3.0",
"@testing-library/dom": "^8.0.0",
"@testing-library/jest-dom": "^5.14.1",
"@testing-library/react": "^13.2.0",
"@testing-library/dom": "^9.2.0",
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/react": "^14.0.0",
"@testing-library/user-event": "^12.1.3",
"@types/react": "^17.0.37",
"@types/storybook__react": "^5.2.1",
Expand All @@ -109,26 +109,25 @@
"css-parse": "^2.0.0",
"delta-e": "^0.0.8",
"diff": "^5.1.0",
"eslint": "^8.25.0",
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-jest": "^27.1.1",
"eslint-plugin-jsdoc": "^39.3.6",
"eslint-plugin-jsx-a11y": "^6.6.1",
"eslint": "^8.38.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-jest": "^27.2.1",
"eslint-plugin-jsdoc": "^40.3.0",
"eslint-plugin-jsx-a11y": "^6.7.1",
"eslint-plugin-monorepo": "^0.3.2",
"eslint-plugin-react": "^7.31.10",
"eslint-plugin-react": "^7.32.2",
"eslint-plugin-react-hooks": "^4.6.0",
"eslint-plugin-rulesdir": "^0.2.1",
"eslint-plugin-rulesdir": "^0.2.2",
"fast-check": "^2.19.0",
"fast-glob": "^3.1.0",
"fs-extra": "^10.0.0",
"full-icu": "^1.3.0",
"identity-obj-proxy": "^3.0.0",
"ignore-styles": "^5.0.1",
"jest": "^27.4.3",
"jest-environment-jsdom": "^27.4.3",
"jest-junit": "^12.0.0",
"jest-matchmedia-mock": "^1.0.0",
"jsdom": "^16.7.0",
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

included by jest-environment-jsdom

"jest": "^29.5.0",
"jest-environment-jsdom": "^29.5.0",
"jest-junit": "^15.0.0",
"jest-matchmedia-mock": "^1.1.0",
"lerna": "^3.13.2",
"lfcdn": "^0.4.2",
"md5": "^2.2.1",
Expand Down Expand Up @@ -174,7 +173,6 @@
"resolutions": {
"@babel/core": "7.12.10",
"postcss-calc": "6.0.2",
"jsdom": "16.7.0",
"postcss-modules": "^3.2.2",
"react-refresh": "0.9.0",
"browserslist": "4.20.3"
Expand Down
3 changes: 1 addition & 2 deletions packages/@react-aria/calendar/test/useCalendar.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ import React from 'react';

describe('useCalendar', () => {
beforeAll(() => {
jest.useFakeTimers('legacy');
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
jest.useFakeTimers();
});

afterEach(() => {
Expand Down
14 changes: 7 additions & 7 deletions packages/@react-aria/checkbox/test/useCheckboxGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('useCheckboxGroup', () => {
]} />
);

let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

getByRole only supports strings now

let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
expect(checkboxGroup).toBeInTheDocument();
expect(checkboxes.length).toBe(3);
Expand Down Expand Up @@ -119,7 +119,7 @@ describe('useCheckboxGroup', () => {
{value: 'dragons', children: 'Dragons'}
]} />
);
let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');

let labelId = checkboxGroup.getAttribute('aria-labelledby');
expect(labelId).toBeDefined();
Expand All @@ -137,7 +137,7 @@ describe('useCheckboxGroup', () => {
{value: 'dragons', children: 'Dragons'}
]} />
);
let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');

expect(checkboxGroup).toHaveAttribute('aria-label', 'My Favorite Pet');
});
Expand All @@ -153,7 +153,7 @@ describe('useCheckboxGroup', () => {
{value: 'dragons', children: 'Dragons'}
]} />
);
let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');

expect(checkboxGroup).toHaveAttribute('data-testid', 'favorite-pet');
});
Expand All @@ -171,7 +171,7 @@ describe('useCheckboxGroup', () => {
]} />
);

let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');
expect(checkboxGroup).toHaveAttribute('aria-disabled', 'true');

let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
Expand All @@ -198,7 +198,7 @@ describe('useCheckboxGroup', () => {
]} />
);

let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');
expect(checkboxGroup).not.toHaveAttribute('aria-disabled');

let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
Expand All @@ -218,7 +218,7 @@ describe('useCheckboxGroup', () => {
]} />
);

let checkboxGroup = getByRole('group', {exact: true});
let checkboxGroup = getByRole('group');
expect(checkboxGroup).not.toHaveAttribute('aria-disabled');

let checkboxes = getAllByRole('checkbox') as HTMLInputElement[];
Expand Down
5 changes: 1 addition & 4 deletions packages/@react-aria/color/test/useColorWheel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,7 @@ describe('useColorWheel', () => {
let onChangeSpy = jest.fn();

beforeAll(() => {
// @ts-ignore
jest.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => cb());
// @ts-ignore
jest.useFakeTimers('legacy');
jest.useFakeTimers();
});

afterEach(() => {
Expand Down
10 changes: 10 additions & 0 deletions packages/@react-aria/dnd/src/useAutoScroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ export function useAutoScroll(ref: RefObject<Element>) {
dy: 0
}).current;

useEffect(() => {
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

saw we missed a raf cleanup

Copy link
Member

@LFDanLu LFDanLu May 6, 2023

Choose a reason for hiding this comment

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

for other reviewers, noted that TableView DnD doesn't auto scroll but that seems to have been broken since before this PR

return () => {
if (state.timer) {
cancelAnimationFrame(state.timer);
state.timer = null;
}
};
// state will become a new object, so it's ok to use in the dependency array for unmount
}, [state]);

let scroll = useCallback(() => {
scrollableRef.current.scrollLeft += state.dx;
scrollableRef.current.scrollTop += state.dy;
Expand Down
3 changes: 1 addition & 2 deletions packages/@react-aria/dnd/test/useDraggableCollection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import userEvent from '@testing-library/user-event';

describe('useDraggableCollection', () => {
beforeEach(() => {
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0));
jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(() => ({
left: 0,
top: 0,
Expand All @@ -33,7 +32,7 @@ describe('useDraggableCollection', () => {
height: 50
}));

jest.useFakeTimers('legacy');
jest.useFakeTimers();
});

afterEach(() => {
Expand Down
19 changes: 9 additions & 10 deletions packages/@react-aria/dnd/test/useDroppableCollection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import userEvent from '@testing-library/user-event';

describe('useDroppableCollection', () => {
beforeEach(() => {
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0));
jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function () {
let y = 0;
let height = 50;
Expand Down Expand Up @@ -63,7 +62,7 @@ describe('useDroppableCollection', () => {
return this.getBoundingClientRect().height;
});

jest.useFakeTimers('legacy');
jest.useFakeTimers();
});

afterEach(() => {
Expand Down Expand Up @@ -224,35 +223,35 @@ describe('useDroppableCollection', () => {

let dataTransfer = new DataTransfer();
fireEvent(draggable, new DragEvent('dragstart', {dataTransfer, clientX: 0, clientY: 0}));
act(() => jest.runAllTimers());
act(() => jest.advanceTimersToNextTimer());
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

rafs appear to be able to cause an infinite timer loop, so move all of these to advanceTimersToNextTimer, since that's all we actually want to do here anyways

expect(draggable).toHaveAttribute('data-dragging', 'true');

fireEvent(cells[0], new DragEvent('dragenter', {dataTransfer, clientX: 30, clientY: 30}));
act(() => jest.runOnlyPendingTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).not.toHaveBeenCalled();

fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 100}));
act(() => jest.runOnlyPendingTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).not.toHaveBeenCalled();

fireEvent(cells[4], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 135}));
act(() => jest.runOnlyPendingTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).toHaveBeenCalledTimes(1);
act(() => jest.runOnlyPendingTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).toHaveBeenCalledTimes(2);
jest.clearAllTimers();

fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 100}));
act(() => jest.runAllTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).toHaveBeenCalledTimes(2);

fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 15}));
act(() => jest.runOnlyPendingTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).toHaveBeenCalledTimes(3);
jest.clearAllTimers();

fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 30}));
act(() => jest.runAllTimers());
act(() => jest.advanceTimersToNextTimer());
expect(scrollTop).toHaveBeenCalledTimes(3);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/grid/test/useGrid.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function renderGrid(props = {}) {

describe('useGrid', () => {
beforeAll(() => {
jest.useFakeTimers('modern');
jest.useFakeTimers();
});
afterEach(() => {
// run out notifications
Expand Down
7 changes: 2 additions & 5 deletions packages/@react-aria/interactions/test/useHover.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function pointerEvent(type, opts) {

describe('useHover', function () {
beforeAll(() => {
jest.useFakeTimers('legacy');
jest.useFakeTimers();
});

it('does not handle hover events if disabled', function () {
Expand Down Expand Up @@ -530,10 +530,7 @@ describe('useHover', function () {
}

beforeAll(() => {
jest.useFakeTimers('legacy');
});
afterAll(() => {
jest.useRealTimers();
jest.useFakeTimers();
Copy link
Member

@ktabors ktabors Apr 29, 2023

Choose a reason for hiding this comment

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

Why do we have to have jest.useFakeTimers in this beforeAll()? The above beforeAll() is scoped to cover this abnd has jest.useFakeTimers.

Copy link
Member Author

@snowystinger snowystinger Apr 30, 2023

Choose a reason for hiding this comment

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

no idea, didn't notice it, could probably be removed

});

let matchMedia;
Expand Down
3 changes: 1 addition & 2 deletions packages/@react-aria/interactions/test/useLongPress.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ function ExampleWithPress(props) {

describe('useLongPress', function () {
beforeAll(() => {
jest.useFakeTimers('legacy');
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
jest.useFakeTimers();
});

afterEach(() => {
Expand Down
8 changes: 1 addition & 7 deletions packages/@react-aria/interactions/test/useMove.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,7 @@ function Example(props) {

describe('useMove', function () {
beforeAll(() => {
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
jest.useFakeTimers('legacy');
});

afterEach(() => {
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

duplicate afterEach, can safely remove one of them

// for restoreTextSelection
jest.runAllTimers();
jest.useFakeTimers();
});

afterEach(() => {
Expand Down
15 changes: 4 additions & 11 deletions packages/@react-aria/interactions/test/usePress.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ function pointerEvent(type, opts) {

describe('usePress', function () {
beforeAll(() => {
jest.useFakeTimers('legacy');
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
jest.useFakeTimers();
});

afterEach(() => {
Expand Down Expand Up @@ -2441,7 +2440,7 @@ describe('usePress', function () {
fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]});
fireEvent.touchEnd(el, {changedTouches: [{identifier: 1}]});

act(() => {jest.advanceTimersByTime(300);});
act(() => {jest.advanceTimersByTime(316);});
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

raf is no longer instant in modern timer mocking, so have to account for it while advancing timers

Copy link
Member

@ktabors ktabors Apr 29, 2023

Choose a reason for hiding this comment

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

why change by 16? That makes me nervous that this is too fragile.

Copy link
Member Author

@snowystinger snowystinger Apr 30, 2023

Choose a reason for hiding this comment

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

a raf takes ~16ms in the real world, in fake timers land, it always takes 16. previously we had a mock of the raf with a timeout of 0
it only broke when we changed our fake timer implementation, I'm not too worried about it being too brittle, it's lasted a long time already

expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect);

// Checkbox doesn't remove `user-select: none;` style from HTML Element issue
Expand Down Expand Up @@ -2611,7 +2610,7 @@ describe('usePress', function () {
expect(document.documentElement.style.webkitUserSelect).toBe('none');

unmount();
act(() => {jest.advanceTimersByTime(300);});
act(() => {jest.advanceTimersByTime(316);});
expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect);
});

Expand Down Expand Up @@ -2698,17 +2697,12 @@ describe('usePress', function () {
}

beforeAll(() => {
jest.useFakeTimers('legacy');
});
afterAll(() => {
jest.useRealTimers();
jest.useFakeTimers();
});

let matchMedia;
beforeEach(() => {
matchMedia = new MatchMediaMock();
// this needs to be a setTimeout so that the dialog can be removed from the dom before the callback is invoked
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(() => cb(), 0));
});

afterEach(() => {
Expand All @@ -2723,7 +2717,6 @@ describe('usePress', function () {
}

matchMedia.clear();
window.requestAnimationFrame.mockRestore();
});

describe.each`
Expand Down
14 changes: 11 additions & 3 deletions packages/@react-aria/overlays/test/useModal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,17 @@ describe('useModal', function () {
</div>
)
).toThrow();
expect(console.error).toHaveBeenCalledWith(
expect.stringContaining('An OverlayContainer must not be inside another container. Please change the portalContainer prop.'),
expect.anything()
expect.extend({
toHaveBeenNthCalledWithError(received, index, arg) {
return {
pass: received.mock.calls[index - 1][0].toString().includes(arg),
message: () => `expected ${received.mock.calls[0][0]} to include ${arg}`
};
}
});
expect(console.error).toHaveBeenNthCalledWithError(
Copy link
Member Author

@snowystinger snowystinger Apr 18, 2023

Choose a reason for hiding this comment

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

error shape changed, add matcher that makes it easy to check

1,
'An OverlayContainer must not be inside another container. Please change the portalContainer prop.'
);
});
});
Expand Down
Loading