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 warnings in the demos #25140

Merged
merged 8 commits into from
Mar 7, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 28, 2021

Fix 4 of the 6 errors existing, what's left:

  1. [Tabs] pseudo class ":first-child" is potentially unsafe #24894
  2. Issue with pickers displaying a warning on the wrong mask pattern

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 28, 2021

No bundle size changes

Generated by 🚫 dangerJS against 21e2d53

@oliviertassinari oliviertassinari marked this pull request as draft February 28, 2021 16:01
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Feb 28, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Feb 28, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Feb 28, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Feb 28, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Feb 28, 2021
@oliviertassinari oliviertassinari force-pushed the test-warnings branch 2 times, most recently from e9fb8ff to fa25748 Compare February 28, 2021 19:35
@oliviertassinari oliviertassinari marked this pull request as ready for review February 28, 2021 19:50
@oliviertassinari
Copy link
Member Author

@eps1lon There is a tradeoff to make between running the visual regression tests in dev mode and having the dev warnings vs. running them in production mode and having them run faster.

In dev mode, it's about 50% slower. I would be leaning toward keeping the production mode enable, but no strong point of view.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

The idea is great but the current implementation will be frustrating to debug with.

test/regressions/index.test.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/regressions/index.test.js Outdated Show resolved Hide resolved
test/regressions/index.test.js Outdated Show resolved Hide resolved
docs/package.json Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2021
@oliviertassinari oliviertassinari requested a review from eps1lon March 2, 2021 23:54
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Let's follow the approach we have for the unit tests and collect all during a test, throw them in afterEach and clear them in beforeEach. That way we don't swallow console calls and make sure tests don't leak if they fail.

test/regressions/index.test.js Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member Author

Let's follow the approach we have for the unit tests and collect all during a test, throw them in afterEach and clear them in beforeEach. That way we don't swallow console calls and make sure tests don't leak if they fail.

Haha, I'm not surprised you asked for this. The docs wasn't covering the API to stop listening to an event. But I might not have searched enough. Ok, I'm on it.

@eps1lon
Copy link
Member

eps1lon commented Mar 3, 2021

The docs wasn't covering the API to stop listening to an event.

I noticed this as well. But it should have (add|remove)Listener i.e. implement EventListener. on|off should work as well but I find their naming a bit obscure.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 3, 2021

I have pushed it further. I have done my best, taking your feedback into account. If it's not good enough. I would propose I simply fix the warnings, and we can come back to adding safety in the CI later. I'm going over the time budget I can spend on this. I have tried using on/off but it wasn't delivering. It seems to create more complexity than solving any problem. For instance, we need to catch the console log that popup even before we start taking screenshots (during bootstrap).

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 5, 2021
@oliviertassinari oliviertassinari changed the title [test] Output warnings in the rendered components [test] Fix warnings in the demos Mar 7, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 7, 2021
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 7, 2021

I have reduced the ambition of the changes. It's about fixing existing warnings in the demos.

For the tests, it was reverted in 21e2d53. If we are happy with the approach, I can replay the commit (I personally think that it's good enough), otherwise for another time.

@oliviertassinari oliviertassinari requested a review from eps1lon March 7, 2021 15:05
@oliviertassinari oliviertassinari merged commit 17143ce into mui:next Mar 7, 2021
@oliviertassinari oliviertassinari deleted the test-warnings branch March 7, 2021 21:09
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.

3 participants