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.

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 failing dynamic row height tests on Edge #5707

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

m4theushw
Copy link
Member

Some tests are failing on Edge after the merge of #4155

@m4theushw m4theushw added the test label Aug 5, 2022
@mui-bot
Copy link

mui-bot commented Aug 5, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 252.4 462.8 373.9 351.98 85.492
Sort 100k rows ms 471.7 857 712.2 685.94 136.136
Select 100k rows ms 148.1 210.3 203 194.42 23.347
Deselect 100k rows ms 114 287.1 146.1 179.46 62.6

Generated by 🚫 dangerJS against a7eef9e

@m4theushw m4theushw marked this pull request as ready for review August 5, 2022 20:48
@m4theushw m4theushw merged commit 474c50e into mui:master Aug 5, 2022
@m4theushw m4theushw deleted the fix-ci-edge branch August 5, 2022 20:49
it('should position correctly the render zone when the 2nd page has less rows than the 1st page', async () => {
it('should position correctly the render zone when the 2nd page has less rows than the 1st page', async function test() {
if (/edg/i.test(window.navigator.userAgent)) {
this.skip(); // FIXME: We need a waitFor that works with fake clock
Copy link
Member

Choose a reason for hiding this comment

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

@m4theushw
This test also fails in Chrome and Firefox sometimes:

https://app.circleci.com/pipelines/github/mui/mui-x/21944/workflows/5b674c92-8ba9-427e-a902-d1a516513cd6/jobs/126695

Firefox 78.0 (Windows 10) <DataGrid /> - Rows prop: getRowHeight dynamic row height should position correctly the render zone when the 2nd page has less rows than the 1st page FAILED
	expected inline style of <div …(2)>…(1)</div> did not match
	Expected:
	{
	  "transform": "translate3d(0px, 0px, 0px)"
	}
	Actual:
	{
	  "transform": "translate3d(0px, 988px, 0px)"
	}

https://app.circleci.com/pipelines/github/mui/mui-x/21943/workflows/4ec09158-5c2c-4fb8-b994-52816ab953e1/jobs/126682

Chrome 87.0.4280.66 (Mac OS 10.15.6) <DataGrid /> - Rows prop: getRowHeight dynamic row height should position correctly the render zone when the 2nd page has less rows than the 1st page FAILED
	AssertionError: expected inline style of <div …(2)></div> did not match
	Expected:
	{
	  "transform": "translate3d(0px, 0px, 0px)"
	}
	Actual:
	{
	  "transform": "translate3d(0px, 1040px, 0px)"
	}

I can reproduce it locally using Chrome instead of ChromeHeadless.
Test fails only when browser window is in background.

diff --git a/test/karma.conf.js b/test/karma.conf.js
index 2ea932985..f40b51137 100644
--- a/test/karma.conf.js
+++ b/test/karma.conf.js
@@ -44,7 +44,7 @@ const MAX_CIRCLE_CI_CONCURRENCY = 83;
 module.exports = function setKarmaConfig(config) {
   const baseConfig = {
     basePath: '../',
-    browsers: ['chromeHeadless'],
+    browsers: ['Chrome'],
     browserDisconnectTimeout: 3 * 60 * 1000, // default 2000
     browserDisconnectTolerance: 1, // default 0
     browserNoActivityTimeout: 6 * 60 * 1000, // default 10000

Any ideas?

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 don't have any clue of why they fail. When I did this PR, I only had problems with Edge so ignoring this browser in this specific test solved the problem. https://app.circleci.com/pipelines/github/mui/mui-x/21728/workflows/12ccc593-a493-414b-9484-79cb96b8a991/jobs/125448 is from a7eef9e

I think we'll need to skip all tests related to dynamic row height since they are too flacky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants