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.

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

Jump to bottom

[test] Replace waitFor() with act() #14851

Merged
merged 6 commits into from
Oct 9, 2024
Merged

[test] Replace waitFor() with act() #14851

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64e41d0
[test] Replace waitFor with act
oliviertassinari Oct 6, 2024
b575fa0
a few more
oliviertassinari Oct 6, 2024
d0b1687
more
oliviertassinari Oct 6, 2024
9ac8dba
[test] Add missing async
oliviertassinari Oct 6, 2024
b1f68d0
more cleanup
oliviertassinari Oct 7, 2024
191aa84
fix ci
oliviertassinari Oct 9, 2024
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: 4 additions & 6 deletions packages/x-data-grid-pro/src/tests/editComponents.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,9 @@ describe('<DataGridPro /> - Edit components', () => {

const input = within(cell).getByRole<HTMLInputElement>('textbox');
fireEvent.change(input, { target: { value: 'Puma' } });
await act(() => Promise.resolve());

expect(onValueChange.callCount).to.equal(1);
expect(onValueChange.lastCall.args[1]).to.equal('Puma');
await act(() => Promise.resolve());
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
});
});

Expand Down Expand Up @@ -338,12 +337,12 @@ describe('<DataGridPro /> - Edit components', () => {

const input = cell.querySelector('input')!;
fireEvent.change(input, { target: { value: '2022-02-10' } });
await act(() => Promise.resolve());

expect(onValueChange.callCount).to.equal(1);
expect((onValueChange.lastCall.args[1]! as Date).toISOString()).to.equal(
new Date(2022, 1, 10).toISOString(),
);
await act(() => Promise.resolve());
});
});

Expand Down Expand Up @@ -576,9 +575,8 @@ describe('<DataGridPro /> - Edit components', () => {
const cell = getCell(0, 0);
fireEvent.doubleClick(cell);
fireUserEvent.mousePress(document.getElementById('outside-grid')!);
await act(() => Promise.resolve());

expect(onCellEditStop.callCount).to.equal(1);
await act(() => Promise.resolve());
});

it('should not open the suggestions when Enter is pressed', async () => {
Expand Down Expand Up @@ -642,7 +640,7 @@ describe('<DataGridPro /> - Edit components', () => {
const input = within(cell).getByRole<HTMLInputElement>('checkbox');
await user.click(input);

await waitFor(() => expect(onValueChange.callCount).to.equal(1));
expect(onValueChange.callCount).to.equal(1);
expect(onValueChange.lastCall.args[1]).to.equal(true);
});
});
Expand Down
2 changes: 0 additions & 2 deletions packages/x-data-grid-pro/src/tests/pagination.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ describe('<DataGridPro /> - Pagination', () => {
act(() => {
apiRef.current.setPage(1);
});

expect(getColumnValues(0)).to.deep.equal(['1']);
});

Expand Down Expand Up @@ -65,7 +64,6 @@ describe('<DataGridPro /> - Pagination', () => {
act(() => {
apiRef.current.setPage(50);
});

expect(getColumnValues(0)).to.deep.equal(['19']);
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ describe('<DataGridPro /> - Row editing', () => {
const processRowUpdate = spy((newRow) => newRow);
render(<TestCase processRowUpdate={processRowUpdate} />);
act(() => apiRef.current.startRowEditMode({ id: 0 }));
await act(() => {
await act(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify why this has been added here? 🤔
There does not seem to be any async action within this act. 🤷

Copy link
Member Author

@oliviertassinari oliviertassinari Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@LukasTy I used to think the same as you: How can this be any different? But I have noticed behavior changes.

If I remove the async in https://github.com/mui/base-ui/blob/463d61a3befa8676e564db695eee2ef645060cec/packages/mui-base/src/Tooltip/Root/TooltipRoot.test.tsx#L103 , it crashes the test, with this error:

The current testing environment is not configured to support act

That error is coming from: https://github.com/facebook/react/blob/1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd/packages/react-reconciler/src/ReactFiberAct.js#L54

I have seen the same in testing-library/react-testing-library#1061 (comment).

Looking at the source, I don't quite understand why: https://github.com/facebook/react/blob/1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd/packages/react/src/ReactAct.js#L109. Maybe it's because we use a fake clock.

@Janpot A preference on this? You flagged this as wrong in testing-library/eslint-plugin-testing-library#915

Copy link
Member

Choose a reason for hiding this comment

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

Nice. This means that I've been fighting with this error for a bit as well. 🤯 🙈
Adding async in the await act also helps me avoid the flaky error in my cases. 👍

Copy link
Member

Choose a reason for hiding this comment

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

My preference is to follow what React suggests, which is to always use the async version as they plan to deprecate the sync version. That's why I opened that feature request. I noticed while fixing act calls in tests (e.g. remove ones that should be unnecessary and wrap calls that should be wrapped) that it magically made tings work more predictably 🙂.

I believe the use of waitFor() should be avoided as much as possible. We want to be able to test changes with incremental steps, so using waitFor feels fundamentally a last resort option.

I somewhat disagree with this statement though. I believe the primary reason (not the only) for having tests is to have a codebase that is optimized for refactoring. The most useful tests in that mindset are the ones that don't need an update when implementation changes. act makes tests rely on implementation details and waitFor takes those assumptions away. Not to say that you can't have any tests that test implementation details, just the bulk are testing behavior and the closer you can get your test to real user interaction, the more useful they become (real user events, wait for a11y selector, screenshots,...). Most of the time comes at a cost though 🙂.

Copy link
Member Author

@oliviertassinari oliviertassinari Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

which is to always use the async version as they plan to deprecate the sync version

@Janpot Agree, I opened mui/material-ui#44028 and mui/base-ui#706 for this reason. But I was wondering about

await act(() => {
// vs.
await act(async () => {

The most useful tests in that mindset are the ones that don't need an update when implementation changes.

I agree with this. My assumption is that for act() to not be enough and having to need waitFor(), it must mean that we are doing something creative, in 90+% of the time, it shouldn't be needed. I would expect, as a developer, to be able to take a component from MUI and write a test with prop changes or act() but without waitFor().

Copy link
Member

@Janpot Janpot Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would expect, as a developer, to be able to take a component from MUI and write a test with prop changes or act() but without waitFor().

I see where you're coming from, but I expect the opposite. The way I see it is that the only reason this works is because we execute those events synchronously. Which is quite far from real world usage. To bring the test closer to real user behavior you'd have to think about it as if you were interacting with the page through the event loop. i.e. executing the .click() only schedules an event on the event loop and then returns, instead the current behavior as if we're actually in control of when the event fires. If that were the behavior, the only way to test whether a change happened is through waitFor. This is much closer to how you'd interact with a real browser, e.g. over the devtools protocol, and therefore much closer to real user interaction.

Copy link
Member Author

@oliviertassinari oliviertassinari Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is much closer to how you'd interact with a real browser, e.g. over the devtools protocol, and therefore much closer to real user interaction.

@Janpot If these were e2e tests, yes, I would expect the same. But those are not e2e tests: we don't fetch data from the network, we don't do real event interactions, so I expect deterministic behavior.

Copy link
Member

Choose a reason for hiding this comment

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

But it's act that introduces the non-determinism. Tests randomly break on patch releases of dependencies, when the implementation slightly changes, when running them in browsers vs. node, When react updates, when passing it an async vs sync function,... I fail to see what this notion of deterministic behavior do for us.

It's not because we use user-event and .waitFor that our tests becomes an e2e-tests. I don't feel like there is much to gain from rigorously categorizing tests this way anyway. It's still a component as a unit that's under test. And it's still testing the exact same behavior. But what it's not doing is alter React behavior in a way that never occurs in the real world. And fire events in isolation that would never fire in isolation in the real world.

Copy link
Member Author

@oliviertassinari oliviertassinari Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tests randomly break on patch releases of dependencies, when the implementation slightly changes, when running them in browsers vs. node, When react updates, when passing it an async vs sync function

Ah, then act() is not doing its job 😀. It didn't used to be like this. It would reliably work.

I fail to see what this notion of deterministic behavior do for us.

For me, it's about, after doing an action, knowing that the state if reflected in the DOM without having to think about a complex selector for it (waitFor), as a developer, I want to use one that will always work (act). Then, I can console.log, inspect the DOM, etc. It will always show me up-to-date information, no stale state.

apiRef.current.setEditCellValue({
id: 0,
field: 'currencyPair',
Expand Down Expand Up @@ -1199,7 +1199,7 @@ describe('<DataGridPro /> - Row editing', () => {
const cell = getCell(0, 2);
fireUserEvent.mousePress(cell);
fireEvent.doubleClick(cell);
await act(() => {
await act(async () => {
apiRef.current.setEditCellValue({ id: 0, field: 'price1M', value: 'USD GBP' });
});
fireEvent.keyDown(cell.querySelector('input')!, { key: 'Tab' });
Expand Down
6 changes: 2 additions & 4 deletions packages/x-data-grid-pro/src/tests/rowPinning.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,16 +872,14 @@ describe('<DataGridPro /> - Row pinning', () => {
expect(getCell(0, 1).textContent).to.equal('Joe');
expect(getCell(4, 1).textContent).to.equal('Cory');

act(() =>
await act(() =>
apiRef.current.updateRows([
{ id: 3, name: 'Marcus' },
{ id: 4, name: 'Tom' },
]),
);

await waitFor(() => {
expect(getCell(0, 1).textContent).to.equal('Marcus');
});
expect(getCell(0, 1).textContent).to.equal('Marcus');
expect(getCell(4, 1).textContent).to.equal('Tom');
});
});
4 changes: 3 additions & 1 deletion packages/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,9 @@ describe('<DataGridPro /> - Rows', () => {
/>,
);
expect(document.querySelectorAll('[role="row"][data-rowindex]')).to.have.length(6);
act(() => apiRef.current.setPage(1));
act(() => {
apiRef.current.setPage(1);
});
expect(document.querySelectorAll('[role="row"][data-rowindex]')).to.have.length(4);
});
});
Expand Down
12 changes: 4 additions & 8 deletions packages/x-data-grid/src/tests/rowSelection.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ describe('<DataGrid /> - Row selection', () => {
expect(getRow(0).querySelector('input')).to.have.property('checked', false);
});

it('should set focus on the cell when clicking the checkbox', async () => {
it('should set focus on the cell when clicking the checkbox', () => {
render(<TestDataGridSelection checkboxSelection />);
expect(getActiveCell()).to.equal(null);

Expand All @@ -237,9 +237,7 @@ describe('<DataGrid /> - Row selection', () => {

fireUserEvent.mousePress(checkboxInput!);

await waitFor(() => {
expect(getActiveCell()).to.equal('0-0');
});
expect(getActiveCell()).to.equal('0-0');
});

it('should select all visible rows regardless of pagination', () => {
Expand Down Expand Up @@ -384,9 +382,7 @@ describe('<DataGrid /> - Row selection', () => {
);
const selectAllCheckbox = screen.getByRole('checkbox', { name: 'Select all rows' });
fireEvent.click(selectAllCheckbox);
await act(() => {
expect(getSelectedRowIds()).to.deep.equal([0, 1, 2, 3]);
});
expect(getSelectedRowIds()).to.deep.equal([0, 1, 2, 3]);
expect(grid('selectedRowCount')?.textContent).to.equal('4 rows selected');

fireEvent.change(screen.getByRole('spinbutton', { name: 'Value' }), {
Expand Down Expand Up @@ -607,7 +603,7 @@ describe('<DataGrid /> - Row selection', () => {
});

describe('prop: isRowSelectable', () => {
it('should update the selected rows when the isRowSelectable prop changes', async () => {
it('should update the selected rows when the isRowSelectable prop changes', () => {
const { setProps } = render(
<TestDataGridSelection isRowSelectable={() => true} checkboxSelection />,
);
Expand Down
10 changes: 5 additions & 5 deletions packages/x-data-grid/src/tests/rowSpanning.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { createRenderer, waitFor, fireEvent, act } from '@mui/internal-test-utils';
import { createRenderer, fireEvent, act } from '@mui/internal-test-utils';
import { expect } from 'chai';
import { DataGrid, useGridApiRef, DataGridProps, GridApi } from '@mui/x-data-grid';
import { getCell, getActiveCell } from 'test/utils/helperFn';
Expand Down Expand Up @@ -224,10 +224,10 @@ describe('<DataGrid /> - Row spanning', () => {
/>,
);
expect(Object.keys(apiRef.current.state.rowSpanning.spannedCells).length).to.equal(0);
apiRef.current.setPage(1);
await waitFor(() =>
expect(Object.keys(apiRef.current.state.rowSpanning.spannedCells).length).to.equal(1),
);
await act(async () => {
apiRef.current.setPage(1);
});
expect(Object.keys(apiRef.current.state.rowSpanning.spannedCells).length).to.equal(1);
expect(Object.keys(apiRef.current.state.rowSpanning.hiddenCells).length).to.equal(1);
});
});
Expand Down
14 changes: 4 additions & 10 deletions packages/x-data-grid/src/tests/sorting.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { createRenderer, fireEvent, screen, act, waitFor } from '@mui/internal-test-utils';
import { createRenderer, fireEvent, screen, act } from '@mui/internal-test-utils';
import { expect } from 'chai';
import {
DataGrid,
Expand Down Expand Up @@ -712,9 +712,7 @@ describe('<DataGrid /> - Sorting', () => {
expect(getColumnValues(1)).to.deep.equal(['Adidas', 'Nike', 'Puma']);

setProps({ columns: [{ field: 'id' }] });
await waitFor(() => {
expect(getColumnValues(0)).to.deep.equal(['0', '1', '2']);
});
expect(getColumnValues(0)).to.deep.equal(['0', '1', '2']);
expect(onSortModelChange.callCount).to.equal(1);
expect(onSortModelChange.lastCall.firstArg).to.deep.equal([]);
});
Expand Down Expand Up @@ -747,9 +745,7 @@ describe('<DataGrid /> - Sorting', () => {
expect(getColumnValues(1)).to.deep.equal(['Adidas', 'Nike', 'Puma']);

setProps({ columns: [{ field: 'id' }], sortModel: [{ field: 'id', sort: 'desc' }] });
await waitFor(() => {
expect(getColumnValues(0)).to.deep.equal(['2', '1', '0']);
});
expect(getColumnValues(0)).to.deep.equal(['2', '1', '0']);
expect(onSortModelChange.callCount).to.equal(0);
});

Expand Down Expand Up @@ -790,9 +786,7 @@ describe('<DataGrid /> - Sorting', () => {

const header = getColumnHeaderCell(0);
fireEvent.click(header);
await waitFor(() => {
expect(getColumnValues(0)).to.deep.equal(['a', 'b', '', '']);
});
expect(getColumnValues(0)).to.deep.equal(['a', 'b', '', '']);

fireEvent.click(header);
expect(getColumnValues(0)).to.deep.equal(['b', 'a', '', '']);
Expand Down