[test] Fix flaky data source tests in DataGrid #16395

lauri865 · 2025-01-29T22:50:34Z

I have followed (at least) the PR section of the contributing guide.

Randomly failing datasource tests have given me so much grief. It seems like there's a timing issue with useMockServer and datasource tests. I'm not 100% sure if I got to the bottom of it, but the problem is somewhere in useMockServer initialisation (doesn't happen synchronously), so every time fetchRows function reference changes, fetchRowSpy gets reset, but there could be timing issues still where previous fetches happen after fetchRowSpy has been reset, but fetch comes in later.

With delayed mounting, and waiting for mockServer, we now have to deal with StrictMode as well, since they don't get reset anymore in the useMemo that also runs twice (at least). Added a monkeypatch for now (a component) to see the actual problematic tests, since I didn't want to meddle with every single test.

Ideally fetchRowSpy should only be reset once per test, or within the test itself, not within the component lifecycle, otherwise these tests can be very flaky. I think for testing purposes, it would be better if mockServer wasn't a hook.

However, now we also have to deal with StrictMode fetches, since with delayed rendering, on mount it will fetch twice (always has, but previously it was reset in useMemo that also runs twice, at least, sometimes more).

Edit: fixed the tests.

mui-bot · 2025-01-29T22:54:30Z

Deploy preview: https://deploy-preview-16395--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 4ca68a2

romgrk · 2025-01-29T23:59:13Z

packages/x-data-grid-premium/src/tests/dataSourceAggregation.DataGridPremium.test.tsx

  let mockServer: ReturnType<typeof useMockServer>;

+  // TODO: Resets strictmode calls, need to find a better fix for this, maybe an AbortController?


I would also be in favor of removing strict mode.

I don't know if you know this, but useGridRegisterPipeProcessor is semi-broken in StrictMode. We can't control whether developers use StrictMode though, so turning it off doesn't just fix it.

StrictMode call – invokes useFirstRender (it will only invoke the callback in StrictMode, since the isFirstRender ref doesn't restore, and we cannot restore it back)

StrictMode call – runs useEffect in useGridRegisterPipeProcessor, that sets isFirstRender to false, and returns cleanup, that will remove the processor at the end of StrictMode invocation

Normal call – useFirstRender has no effect, but processor is cleaned up

Normal call – Processor is re-added only in useEffect, so anything that calls it before (even in useEffect level, but from previous hooks), it fails. Which is the reason why I reordered the hooks in useDatagridPremiumComponents

I'm not sure what a good fix is, besides following the rules of react, and do it in a simple useLayoutEffect to minimise the importance of ordering hooks.

We could prevent returning a cleanup function if isFirstRender is true, but then the processor never gets cleaned up without StrictMode, if it only registers once. Which may or may not be fine.

https://github.com/tumugin/use-disposable/blob/604502d69680b0574c67d185cbd2254493997426/src/useIsStrictMode.ts#L35

MBilalShafi

👍

MBilalShafi · 2025-01-30T07:54:02Z

packages/x-data-grid-premium/src/DataGridPremium/useDataGridPremiumComponent.tsx

  useGridAggregation(apiRef, props);
+  useGridDataSource(apiRef, props);


Fixing this order in this PR to fix another bug, this change might be skipped.

It's fine as long as it comes after useGridAggregation, but if we fix the below, then the order won't matter (for this use case at least):
#16395 (comment)

packages/x-data-grid-premium/src/tests/dataSourceAggregation.DataGridPremium.test.tsx

MBilalShafi · 2025-01-30T08:04:44Z

packages/x-data-grid-generator/src/hooks/useMockServer.ts

@@ -367,5 +368,6 @@ export const useMockServer = (
    loadNewData: () => {
      setIndex((oldIndex) => oldIndex + 1);
    },
+    isReady: Boolean(data?.rows?.length),


It could probably be used in the demos too, let's leave it for later though.
Side note: When doing loadNewData it might not convey the correct information initially since the previous data would be there but outdated, isn't much of a problem though since updating the fetchRows would reset the dataSource retriggering a fetch call anyways.

lauri865 · 2025-01-30T09:49:24Z

@MBilalShafi, I force pushed over the last change, since I fixed the root cause why the test failed – it was a completely valid failure. Throttling was never cleaned up, which caused race conditions when changing models (throttled renderedRowsIntervalChanged was called with stale params after sortModel changed). Try to avoid using greaterThan for fetch calls, that will just allow for mistakes. Using useGridSelector for state selectors was also potentially causing issues, so I refactored those.

Your commit, if you want to reincorporate any of those changes:
bc0dbca

It's not the first time throttling has caused issues, so I suggest creating a custom hook that takes care of cleanups. Easy to forget otherwise with manual implementations each time.

MBilalShafi · 2025-01-30T09:59:45Z

Try to avoid using greaterThan for fetch calls, that will just allow for mistakes.

Absolutely, I also started looking into the root cause, you got it first.
Much appreciated 👍

MBilalShafi

LGTM, let's see if @arminmeh has to add something

arminmeh · 2025-01-30T11:34:27Z

Looks good. Thanks for the improvements
Maybe Reset component could be extracted, but it can also be done later

lauri865 · 2025-01-30T12:39:39Z

Can you please merge please @MBilalShafi? 🙏

github-actions · 2025-01-30T12:47:32Z

Cherry-pick PRs will be created targeting branches: v7.x

arminmeh · 2025-01-30T12:55:49Z

Cherry-pick failed, so I have added the changes manually to #16382

Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>

lauri865 added 3 commits January 29, 2025 21:57

breaking change (pun intended)

e855902

wait for data

f370bad

fix

166a02c

lauri865 added 3 commits January 30, 2025 00:24

fix aggregation with datasource

fcc2765

remove console log

3fa35f8

lint

658ad2b

romgrk reviewed Jan 29, 2025

View reviewed changes

MBilalShafi approved these changes Jan 30, 2025

View reviewed changes

lauri865 added 2 commits January 30, 2025 09:36

fix

2421a70

fix lazy loader

e3985e1

lauri865 force-pushed the break-datasource-tests branch from bc0dbca to e3985e1 Compare January 30, 2025 09:46

lint

c94cecf

lauri865 mentioned this pull request Jan 30, 2025

[DataGridPro] Fetch new rows only once when multiple models are changed in one cycle (@arminmeh) #16382

Merged

MBilalShafi force-pushed the break-datasource-tests branch from 127aa5b to c94cecf Compare January 30, 2025 10:14

Test flakiness

20844c3

lauri865 mentioned this pull request Jan 30, 2025

[data grid] disable processor cleanups in development / testing #16400

Open

1 task

MBilalShafi requested a review from arminmeh January 30, 2025 10:43

MBilalShafi approved these changes Jan 30, 2025

View reviewed changes

arminmeh approved these changes Jan 30, 2025

View reviewed changes

arminmeh changed the title ~~[data grid] Fix flaky data source tests~~ [test] Fix flaky data source tests in DataGrid Jan 30, 2025

arminmeh added test component: data grid This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Jan 30, 2025

Merge branch 'master' into break-datasource-tests

4ca68a2

arminmeh merged commit 1ea3ae2 into mui:master Jan 30, 2025
18 checks passed

arminmeh added a commit to arminmeh/mui-x that referenced this pull request Jan 30, 2025

Cherry-pick changes from mui#16395

aecda75

LukasTy mentioned this pull request Feb 3, 2025

[test] Fix flaky browser tests #16179

Closed

A-s-h-o-k pushed a commit to A-s-h-o-k/mui-x that referenced this pull request Feb 4, 2025

[test] Fix flaky data source tests in DataGrid (mui#16395)

c7f575e

Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Fix flaky data source tests in DataGrid #16395

[test] Fix flaky data source tests in DataGrid #16395

lauri865 commented Jan 29, 2025 •

edited

Loading

mui-bot commented Jan 29, 2025 •

edited

Loading

romgrk Jan 29, 2025

lauri865 Jan 30, 2025 •

edited

Loading

lauri865 Jan 30, 2025

MBilalShafi left a comment

MBilalShafi Jan 30, 2025

lauri865 Jan 30, 2025

MBilalShafi Jan 30, 2025

lauri865 commented Jan 30, 2025 •

edited

Loading

MBilalShafi commented Jan 30, 2025 •

edited

Loading

MBilalShafi left a comment

arminmeh commented Jan 30, 2025

lauri865 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

arminmeh commented Jan 30, 2025

		let mockServer: ReturnType<typeof useMockServer>;

		// TODO: Resets strictmode calls, need to find a better fix for this, maybe an AbortController?

		useGridAggregation(apiRef, props);
		useGridDataSource(apiRef, props);

[test] Fix flaky data source tests in DataGrid #16395

[test] Fix flaky data source tests in DataGrid #16395

Conversation

lauri865 commented Jan 29, 2025 • edited Loading

mui-bot commented Jan 29, 2025 • edited Loading

romgrk Jan 29, 2025

Choose a reason for hiding this comment

lauri865 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

lauri865 Jan 30, 2025

Choose a reason for hiding this comment

MBilalShafi left a comment

Choose a reason for hiding this comment

MBilalShafi Jan 30, 2025

Choose a reason for hiding this comment

lauri865 Jan 30, 2025

Choose a reason for hiding this comment

MBilalShafi Jan 30, 2025

Choose a reason for hiding this comment

lauri865 commented Jan 30, 2025 • edited Loading

MBilalShafi commented Jan 30, 2025 • edited Loading

MBilalShafi left a comment

Choose a reason for hiding this comment

arminmeh commented Jan 30, 2025

lauri865 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

arminmeh commented Jan 30, 2025

lauri865 commented Jan 29, 2025 •

edited

Loading

mui-bot commented Jan 29, 2025 •

edited

Loading

lauri865 Jan 30, 2025 •

edited

Loading

lauri865 commented Jan 30, 2025 •

edited

Loading

MBilalShafi commented Jan 30, 2025 •

edited

Loading