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.

Sign up for GitHub

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

Jump to bottom

[code-infra] Skip charts animation for visual regression test #42530

Merged
merged 4 commits into from
Jun 5, 2024
Merged

[code-infra] Skip charts animation for visual regression test #42530

merged 4 commits into from
Jun 5, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jun 5, 2024 •
edited by oliviertassinari
Loading

Should prevent that type of false positive

image

I also noticed we do screenshot of the dashboard and each component individually. Which seems like waisting resources. So I only kept the dashboard screenshot and remove all the subcomponents

See: #41757 (comment)

@mui-bot
Copy link

mui-bot commented Jun 5, 2024 •
edited
Loading

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 07b9fed

michaldudak
michaldudak reviewed Jun 5, 2024
View reviewed changes
test/regressions/index.js Outdated
@@ -27,7 +33,22 @@ importRegressionFixtures.keys().forEach((path) => {
}, []);

const blacklist = [
'docs-getting-started-templates-dashboard-internals-components/CustomIcons.png', // No public components
// Next components are tested by docs-getting-started-templates-dashboard-components/MainGrid.png
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Next components are tested by docs-getting-started-templates-dashboard-components/MainGrid.png
// The following components are tested by docs-getting-started-templates-dashboard-components/MainGrid.png

The downside is that this list must be manually synchronized with the demos. But it's still better than running all demos unnecessarily, I guess.

mnajdova
mnajdova approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@mnajdova mnajdova 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, thanks!

@alexfauquette alexfauquette merged commit f0026ad into mui:next Jun 5, 2024
19 checks passed
@zannager zannager added the core Infrastructure work going on behind the scenes label Jun 5, 2024
@Janpot
Copy link
Member

Janpot commented Jun 5, 2024 •
edited
Loading

Have you tried disabling animations in the playwright screenshot function here? This is how we do it in Toolpad, I believe it's a relatively new playwright API. The method would apply to all screenshots (including that flaky dashboard one 🙂 and doesn't require messing with the internal implementation details)

@michaldudak
Copy link
Member

If the animation is handled by React Spring, I'm afraid configuring it in Playwright may not be enough.

@alexfauquette
Copy link
Member Author

If the browser settings ask to avoid animation (mostly for a11y issues) the charts will skip them https://mui.com/x/react-charts/pie/#animation

maybe that's what make it works

@Janpot
Copy link
Member

Janpot commented Jun 5, 2024 •
edited
Loading

Right, I'm also setting reducedMotion then in #42537

@oliviertassinari oliviertassinari added test scope: code-infra Specific to the core-infra product labels Jun 9, 2024
@oliviertassinari oliviertassinari changed the title [core] Skip charts animation for visual regression test [code-infra] Skip charts animation for visual regression test Jun 9, 2024
This was referenced Jun 9, 2024
Merged
Merged
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
@alexfauquette @joserodolfofreitas
[core] Skip charts animation for visual regression test (mui#42530)
a1d9bc3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak left review comments

@mnajdova mnajdova mnajdova approved these changes

@zanivan zanivan Awaiting requested review from zanivan

Assignees
No one assigned
Labels
core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alexfauquette @mui-bot @Janpot @michaldudak @mnajdova @oliviertassinari @zannager