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

[core] Remove Suspense and clock mocking from regressions and e2e tests #44935

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jan 3, 2025

Closes: #44920

Changes:

@DiegoAndai DiegoAndai added the core Infrastructure work going on behind the scenes label Jan 3, 2025
@DiegoAndai DiegoAndai self-assigned this Jan 3, 2025
@mui-bot
Copy link

mui-bot commented Jan 3, 2025

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 05a9ec4

@DiegoAndai
Copy link
Member Author

The latest run indicates this is related to changes in Suspense. When removing the Suspense barrier, the regression test goes back to ~4m runtime.

@DiegoAndai DiegoAndai marked this pull request as ready for review January 8, 2025 13:46
@DiegoAndai DiegoAndai requested a review from Janpot January 8, 2025 13:46
@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jan 8, 2025

About the broken Argos test:

On another note: Should we also remove Suspense from the e2e TestViewer?

<React.Suspense fallback={<div aria-busy />}>

This test doesn't present such a difference (React 18 28s and React 19 36s in #44868), but it might be due to it only having 11 tests.

@DiegoAndai DiegoAndai changed the title [core] Test removing suspense from TestViewer [core] Remove suspense from TestViewer Jan 8, 2025
@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jan 8, 2025

I reviewed the screenshots before the React 19 bump and they had 2014, not 2025. This is very weird to me, the year comes from {new Date().getFullYear()} (example). Do you know how were we getting 2014 before Janpot?

Answer: We're mocking the clock:

const clock = useFakeTimers({
now: new Date('Mon Aug 18 14:11:54 2014 -0500'),
toFake: ['Date'],
});

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jan 8, 2025

Should we also remove Suspense from the e2e TestViewer?

Confirmed that the e2e tests also shows the 300s throttling, removing

@DiegoAndai DiegoAndai changed the title [core] Remove suspense from TestViewer [core] Remove Suspense and clock mocking from regressions and e2e tests Jan 8, 2025
@DiegoAndai
Copy link
Member Author

Ok, ready for review @Janpot

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Ok for me, we could live with the yearly flakey test. Alternatively, the following may work in the screenshot call to mask the copyright text:

mask: page.getByText(/Copyright © Sitemark \d\d\d\d\./)

@DiegoAndai DiegoAndai merged commit 0332535 into mui:master Jan 8, 2025
19 checks passed
@DiegoAndai DiegoAndai deleted the regressions-performance branch January 8, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Regression test is 5 times slower after React 19 upgrade
3 participants