Skip to content

[test] Fix flaky data source aggregation test (take 3) #17316

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

Merged

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Apr 10, 2025

@LukasTy LukasTy added test component: data grid This is the name of the generic UI component, not the React module! maintenance labels Apr 10, 2025
@LukasTy LukasTy requested a review from a team April 10, 2025 20:07
@LukasTy LukasTy self-assigned this Apr 10, 2025
@mui-bot
Copy link

mui-bot commented Apr 10, 2025

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

Generated by 🚫 dangerJS against 9b6fccc

@@ -104,6 +104,8 @@ describeSkipIf(isJSDOM)('<DataGridPremium /> - Data source aggregation', () => {
expect(fetchRowsSpy.callCount).to.be.greaterThan(0);
});
await user.click(within(getColumnHeaderCell(0)).getByLabelText('id column menu'));
// wait for the column menu to be open first
await screen.findByRole('menu', { name: 'id column menu' });
Copy link
Member

Choose a reason for hiding this comment

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

What a nasty test to fix 😅
Do you think this will make any difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm no longer sure... 🤷
It's hard to debug it when locally it's impossible to get a failure. 🙈

But at least on Pickers, I often tend to first assert that a Popper has been attached and only then do the actual assertion. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

The popper tests are extremely frustrating to test. There is no exact reason why they fail as far as I could tell

Copy link
Member Author

Choose a reason for hiding this comment

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

And it failed once again... 🤷

@LukasTy
Copy link
Member Author

LukasTy commented Apr 14, 2025

@MBilalShafi thank you for pitching it. 🙏
Do you think the PR makes sense to merge or is there still room for improvement? 🤔

However, I haven't seen this test acting flaky lately, maybe we should wait for an indicator that it is still flaky..? 🤷

@MBilalShafi
Copy link
Member

MBilalShafi commented Apr 14, 2025

I guess it doesn't do any harm to merge it as it could reproduce again since the factor causing flakiness ("async" behavior of useDemoData) isn't fully fixed yet. Ideally, the testing should have a "sync" counterpart of the useDemoData.

@LukasTy
Copy link
Member Author

LukasTy commented Apr 14, 2025

I guess it doesn't do any harm to merge it as it could reproduce again since the factor causing flakiness (async behavior of useDemoData) isn't fully fixed yet.

Gotcha. Could I get approval on the PR? 🤔

@LukasTy LukasTy enabled auto-merge (squash) April 14, 2025 09:35
@LukasTy LukasTy merged commit edec2bd into mui:master Apr 14, 2025
19 checks passed
@LukasTy LukasTy deleted the attempt-to-fix-flaky-data-grid-aggregation-test branch April 14, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! maintenance test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants