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] Update font-awesome CSS file in regression tests fixture #43745

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Sep 13, 2024

Attempt at stabilizing the icons test. It looks like the referenced css has gone out of sync with the regression tests font loader.

Let's see if this does the trick.

Potentially closes #43684

@Janpot Janpot added the test label Sep 13, 2024
@mui-bot
Copy link

mui-bot commented Sep 13, 2024

Netlify deploy preview

https://deploy-preview-43745--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 25d52a4

@Janpot Janpot marked this pull request as ready for review September 13, 2024 12:49
@Janpot Janpot requested a review from aarongarciah September 13, 2024 12:50
@aarongarciah
Copy link
Member

cc @DiegoAndai who's working on #43684

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Looks good, but leaving the approval to @DiegoAndai in case he has some thoughts.

@DiegoAndai
Copy link
Member

@Janpot, thanks for taking this!

To clarify, AFAIU, the flaky tests are the Pigment CSS regression ones added here: #43280

On the pigment fixtures, we have some cases of the icons disappearing: https://app.argos-ci.com/mui/material-ui/builds/31895/107984073

And some with the icons appearing: https://app.argos-ci.com/mui/material-ui/builds/31900/108029355

So, my thought was that network latency was involved. Is that not the case?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 13, 2024

On the pigment fixtures, we have some cases of the icons disappearing: https://app.argos-ci.com/mui/material-ui/builds/31895/107984073 And some with the icons appearing: https://app.argos-ci.com/mui/material-ui/builds/31900/108029355 So, my thought was that network latency was involved. Is that not the case?

@DiegoAndai It seems to match with this PR. We were not waiting for the icon font to load before taking the screenshot.

@Janpot
Copy link
Member Author

Janpot commented Sep 16, 2024

So, my thought was that network latency was involved. Is that not the case?

Yes, there's a race condition between the font loading and the moment the screenshot is captured. I noticed there was setup code in the tests that waits until all fonts are loaded before actually capturing the screenshot. Only, it seems like it's waiting for a different CSS file than the one referenced in the test.

I didn't deeply test this, I just corrected the faulty url. It looks like the screenshot contains the icons, so it worked at least once. If this doesn't fix the flakeyness, I plan to debug this fully, it just didn't feel justified to spend more time on it at the moment since this is a clear bug that could explain the flakeyness, with low risk of breakage, we will notice soon enough if it didn't work.

@Janpot Janpot merged commit 1841de5 into mui:master Sep 16, 2024
19 of 20 checks passed
@Janpot Janpot deleted the fa-load branch September 16, 2024 16:01
@Janpot
Copy link
Member Author

Janpot commented Sep 16, 2024

Seems to not have worked https://app.argos-ci.com/mui/material-ui/builds/32164/109227549

@DiegoAndai
Copy link
Member

@Janpot, my take is that we should remove them.

@Janpot
Copy link
Member Author

Janpot commented Sep 17, 2024

@Janpot, my take is that we should remove them.

🙂 Nah, let's just fix it

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.

5 participants