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

[test] Fix flaky tests #16257

Merged
merged 2 commits into from
Jan 20, 2025
Merged

[test] Fix flaky tests #16257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c28236
fix flaky tests
lauri865 Jan 19, 2025
d7c5b5f
Update packages/x-date-pickers/src/DateCalendar/tests/localization.Da…
lauri865 Jan 20, 2025
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
29 changes: 6 additions & 23 deletions packages/x-data-grid-pro/src/tests/columnPinning.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,7 @@ import {
GridColDef,
} from '@mui/x-data-grid-pro';
import { useBasicDemoData } from '@mui/x-data-grid-generator';
import {
createRenderer,
fireEvent,
screen,
createEvent,
act,
waitFor,
} from '@mui/internal-test-utils';
import { createRenderer, fireEvent, screen, createEvent, act } from '@mui/internal-test-utils';
import {
$,
$$,
Expand Down Expand Up @@ -469,9 +462,7 @@ describe('<DataGridPro /> - Column pinning', () => {
const menuIconButton = columnCell.querySelector('button[aria-label="Menu"]')!;
await user.click(menuIconButton);
await user.click(screen.getByRole('menuitem', { name: 'Pin to left' }));
await waitFor(() => {
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).not.to.equal(null);
});
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).not.to.equal(null);
});

it('should pin the column to the right when clicking the "Pin to right" pinning button', async () => {
Expand All @@ -480,9 +471,7 @@ describe('<DataGridPro /> - Column pinning', () => {
const menuIconButton = columnCell.querySelector('button[aria-label="Menu"]')!;
await user.click(menuIconButton);
await user.click(screen.getByRole('menuitem', { name: 'Pin to right' }));
await waitFor(() => {
expect($(`.${gridClasses['cell--pinnedRight']}[data-field="id"]`)).not.to.equal(null);
});
expect($(`.${gridClasses['cell--pinnedRight']}[data-field="id"]`)).not.to.equal(null);
});

it('should allow to invert the side when clicking on "Pin to right" pinning button on a left pinned column', async () => {
Expand All @@ -491,9 +480,7 @@ describe('<DataGridPro /> - Column pinning', () => {
const menuIconButton = columnCell.querySelector('button[aria-label="Menu"]')!;
await user.click(menuIconButton);
await user.click(screen.getByRole('menuitem', { name: 'Pin to right' }));
await waitFor(() => {
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).to.equal(null);
});
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).to.equal(null);
expect($(`.${gridClasses['cell--pinnedRight']}[data-field="id"]`)).not.to.equal(null);
});

Expand All @@ -503,9 +490,7 @@ describe('<DataGridPro /> - Column pinning', () => {
const menuIconButton = columnCell.querySelector('button[aria-label="Menu"]')!;
await user.click(menuIconButton);
await user.click(screen.getByRole('menuitem', { name: 'Pin to left' }));
await waitFor(() => {
expect($(`.${gridClasses['cell--pinnedRight']}[data-field="id"]`)).to.equal(null);
});
expect($(`.${gridClasses['cell--pinnedRight']}[data-field="id"]`)).to.equal(null);
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).not.to.equal(null);
});

Expand All @@ -515,9 +500,7 @@ describe('<DataGridPro /> - Column pinning', () => {
const menuIconButton = columnCell.querySelector('button[aria-label="Menu"]')!;
await user.click(menuIconButton);
await user.click(screen.getByRole('menuitem', { name: 'Unpin' }));
await waitFor(() => {
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).to.equal(null);
});
expect($(`.${gridClasses['cell--pinnedLeft']}[data-field="id"]`)).to.equal(null);
});

describe('with fake timers', () => {
Expand Down
2 changes: 0 additions & 2 deletions packages/x-data-grid/src/hooks/features/columnMenu/useGridColumnMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export const useGridColumnMenu = (apiRef: React.RefObject<GridPrivateApiCommunit
};
});
apiRef.current.hidePreferences();
apiRef.current.forceUpdate();
}
},
[apiRef, logger],
Expand Down Expand Up @@ -93,7 +92,6 @@ export const useGridColumnMenu = (apiRef: React.RefObject<GridPrivateApiCommunit
columnMenu: newState,
};
});
apiRef.current.forceUpdate();
}
}, [apiRef, logger]);

Expand Down
5 changes: 1 addition & 4 deletions packages/x-data-grid/src/hooks/features/dimensions/useGridDimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import { getTotalHeaderHeight } from '../columns/gridColumnsUtils';
import { GridStateInitializer } from '../../utils/useGridInitializeState';
import { DATA_GRID_PROPS_DEFAULT_VALUES } from '../../../constants/dataGridPropsDefaultValues';
import { isJSDOM } from '../../../utils/isJSDOM';

type RootProps = Pick<
DataGridProcessedProps,
Expand Down Expand Up @@ -327,10 +328,6 @@ export function useGridDimensions(
const handleResize = React.useCallback<GridEventListener<'resize'>>(
(size) => {
rootDimensionsRef.current = size;

// jsdom has no layout capabilities
const isJSDOM = /jsdom|HappyDOM/.test(window.navigator.userAgent);

if (size.height === 0 && !errorShown.current && !props.autoHeight && !isJSDOM) {
logger.error(
[
Expand Down
48 changes: 11 additions & 37 deletions packages/x-data-grid/src/hooks/features/preferencesPanel/useGridPreferencesPanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,42 +24,27 @@ export const useGridPreferencesPanel = (
): void => {
const logger = useGridLogger(apiRef, 'useGridPreferencesPanel');

const hideTimeout = React.useRef<ReturnType<typeof setTimeout>>(undefined);
const immediateTimeout = React.useRef<ReturnType<typeof setTimeout>>(undefined);

/**
* API METHODS
*/
const hidePreferences = React.useCallback(() => {
logger.debug('Hiding Preferences Panel');
const preferencePanelState = gridPreferencePanelStateSelector(apiRef.current.state);
if (preferencePanelState.openedPanelValue) {
apiRef.current.setState((state) => {
if (!state.preferencePanel.open) {
return state;
}

logger.debug('Hiding Preferences Panel');
const preferencePanelState = gridPreferencePanelStateSelector(state);
apiRef.current.publishEvent('preferencePanelClose', {
openedPanelValue: preferencePanelState.openedPanelValue,
});
}
apiRef.current.setState((state) => ({ ...state, preferencePanel: { open: false } }));
apiRef.current.forceUpdate();
return { ...state, preferencePanel: { open: false } };
});
}, [apiRef, logger]);

// This is to prevent the preferences from closing when you open a select box or another panel,
// The issue is in MUI core V4 => Fixed in V5
const doNotHidePanel = React.useCallback(() => {
immediateTimeout.current = setTimeout(() => clearTimeout(hideTimeout.current), 0);
}, []);

// This is a hack for the issue with Core V4, by delaying hiding the panel on the clickAwayListener,
// we can cancel the action if the trigger element still need the panel...
const hidePreferencesDelayed = React.useCallback<
GridPreferencesPanelApi['hidePreferences']
>(() => {
hideTimeout.current = setTimeout(hidePreferences, 100);
}, [hidePreferences]);

const showPreferences = React.useCallback<GridPreferencesPanelApi['showPreferences']>(
(newValue, panelId, labelId) => {
logger.debug('Opening Preferences Panel');
doNotHidePanel();
apiRef.current.setState((state) => ({
...state,
preferencePanel: {
Expand All @@ -73,16 +58,15 @@ export const useGridPreferencesPanel = (
apiRef.current.publishEvent('preferencePanelOpen', {
openedPanelValue: newValue,
});
apiRef.current.forceUpdate();
},
[logger, doNotHidePanel, apiRef],
[logger, apiRef],
);

useGridApiMethod(
apiRef,
{
showPreferences,
hidePreferences: hidePreferencesDelayed,
hidePreferences,
},
'public',
);
Expand Down Expand Up @@ -131,14 +115,4 @@ export const useGridPreferencesPanel = (

useGridRegisterPipeProcessor(apiRef, 'exportState', stateExportPreProcessing);
useGridRegisterPipeProcessor(apiRef, 'restoreState', stateRestorePreProcessing);

/**
* EFFECTS
*/
React.useEffect(() => {
return () => {
clearTimeout(hideTimeout.current);
clearTimeout(immediateTimeout.current);
};
}, []);
};
10 changes: 1 addition & 9 deletions packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import { gridRowSpanningHiddenCellsOriginMapSelector } from '../rows/gridRowSpan
import { gridListColumnSelector } from '../listView/gridListViewSelectors';
import { minimalContentHeight } from '../rows/gridRowsUtils';
import { gridFocusedVirtualCellSelector } from './gridFocusedVirtualCellSelector';
import { isJSDOM } from '../../../utils/isJSDOM';

const MINIMUM_COLUMN_WIDTH = 50;

Expand Down Expand Up @@ -88,15 +89,6 @@ const createScrollCache = (
});
type ScrollCache = ReturnType<typeof createScrollCache>;

let isJSDOM = false;
try {
if (typeof window !== 'undefined') {
isJSDOM = /jsdom|HappyDOM/.test(window.navigator.userAgent);
}
} catch (_) {
/* ignore */
}

export const useGridVirtualScroller = () => {
const apiRef = useGridPrivateApiContext() as React.RefObject<PrivateApiWithInfiniteLoader>;
const rootProps = useGridRootProps();
Expand Down
2 changes: 2 additions & 0 deletions packages/x-data-grid/src/utils/isJSDOM.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export const isJSDOM =
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the try/catch here as it was in useGridVirtualScroller so it doesn't fail in server env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would it fail in server environment? Besides window not being defined, which it checks for.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice there was a check for window alread 👍🏻

typeof window !== 'undefined' && /jsdom|HappyDOM/.test(window.navigator.userAgent);
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 6 additions & 3 deletions packages/x-date-pickers/src/DateCalendar/tests/localization.DateCalendar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { screen, createRenderer } from '@mui/internal-test-utils';
import { DateCalendar, dayCalendarClasses } from '@mui/x-date-pickers/DateCalendar';
import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { createPickerRenderer, AdapterName, availableAdapters } from 'test/utils/pickers';
import { he, fr } from 'date-fns/locale';
import { he, fr, enUS } from 'date-fns/locale';
import 'dayjs/locale/he';
import 'dayjs/locale/fr';
import 'moment/locale/he';
Expand All @@ -21,7 +21,7 @@ describe('<DateCalendar /> - localization', () => {
});

it('should display correct week day labels in Hebrew locale ', () => {
render(<DateCalendar />);
render(<DateCalendar reduceAnimations />);

expect(screen.getByText('א')).toBeVisible();
});
Expand All @@ -31,7 +31,10 @@ describe('<DateCalendar /> - localization', () => {

it('should correctly switch between locale with week starting in Monday and week starting in Sunday', () => {
const { setProps } = renderWithoutWrapper(
<LocalizationProvider dateAdapter={availableAdapters[adapterName]}>
<LocalizationProvider
dateAdapter={availableAdapters[adapterName]}
adapterLocale={adapterName === 'date-fns' ? enUS : 'en-US'}
Copy link
Member

Choose a reason for hiding this comment

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

Was it needed here, because of leaking config from the other test in the suite? 🤔
By default, we shouldn't need to explicitly set the enUS locale as it is the default if none is defined...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Try running the test in isolation, it will fail 100% of the times with moment.js. It's specific to moment.js though, and due to importing side effects that seem to manipulate the locale of the global moment.js instance
import 'moment/locale/fr'; <- this as the last import seems to change the default locale

Maybe we could trigger locale change on top-level for momentjs adapter instead. Or if you know how to fix scoping for momentjs, that would be even better.

Copy link
Contributor Author

@lauri865 lauri865 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It only seems to currently work in CI (and when you run the whole test suite) due to other tests changing the locale back to english. But I have seen the test fail in CI as well, I guess if this test happens to run before the other ones.

>
<DateCalendar reduceAnimations />
</LocalizationProvider>,
);
Expand Down
2 changes: 1 addition & 1 deletion packages/x-date-pickers/src/PickersCalendarHeader/PickersCalendarHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ const PickersCalendarHeader = React.forwardRef(function PickersCalendarHeader(
</SwitchViewButton>
)}
</PickersCalendarHeaderLabelContainer>
<Fade in={view === 'day'}>
<Fade in={view === 'day'} appear={!reduceAnimations} enter={!reduceAnimations}>
Copy link
Member

Choose a reason for hiding this comment

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

Nice find. 👍
But it looks like there is still a hide/show animation when reduceAnimations is specified. 🤔
Does it really remove timeouts?
Have you considered going a similar route as PickersFadeTransitionGroup, where it renders a React.Fragment wrapper when reduceAnimations is true? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd probably the correct way, but I tried to go the easy route to prove the root cause. There's an exit prop as well that should remove the hide animations. Even better might be to put reduceAnimations in a context and have wrappers for all animation components.

Copy link
Member

Choose a reason for hiding this comment

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

That'd probably the correct way, but I tried to go the easy route to prove the root cause.

Gotcha. 👌

There's an exit prop as well that should remove the hide animations.

I tried locally, but it does not seem to change anything, the animation is still there.
If we are going for pure "reduce animations", then there should be no animations when it is toggled, as this prop is mainly an "accessibility" setting.

Even better might be to put reduceAnimations in a context and have wrappers for all animation components.

Great point, now that we have this context, we should be able to move this setting there. 👍

<PickersArrowSwitcher
slots={slots}
slotProps={slotProps}
Expand Down
Loading