refactor storybook and add first keyboard test #36
Conversation
dtassone
changed the title
|
|
||
| test('Cell navigation with arrows ', async done => { | ||
| await page.keyboard.press('ArrowRight'); | ||
| await page.waitFor(100); |
There was a problem hiding this comment.
I think that we should aim to forbid arbitrary sleep durations. Why using 100ms over a different duration?
There was a problem hiding this comment.
Ideally yes. But if I don't put a bit of sleep it will discard the next keypress...
There was a problem hiding this comment.
Should we add a TODO in this case? It sounds like the next keypress shouldn't be discarded, doesn't it impact end-users too?
There was a problem hiding this comment.
No I found a way to remove them I think
| "babel-loader": "^8.1.0", | ||
| "core-js": "^3.6.5", | ||
| "jest": "^26.0.1", | ||
| "jest-image-snapshot": "^4.0.2", | ||
| "jest-puppeteer": "^4.4.0", |
There was a problem hiding this comment.
Haha, guess who's the author of the lib? A friend and former coworker at Doctolib :p.
| @@ -21,7 +21,7 @@ This component has 2 peer dependencies that you will need to install as well. | |||
| ``` | |||
| "peerDependencies": { | |||
| "react": "^16.13.1", | |||
| "styled-components": "^5.0.1" | |||
| "styled-components": "^5.1.0" | |||
There was a problem hiding this comment.
This doesn't seem right.
@material-ui/core already has a CSS-in-JS solution, introducing a second means duplication of bundle size and configuration. I think that we should stick to using @material-ui/styles until we figure out the way out of this with @mnajdova in v5.
Note that it doesn't prevent end-users to use styled-components.
There was a problem hiding this comment.
Styled components is one of the most popular solution now. Css in js is much less practical. If you move away of this framework in v5, I think it is a waste of time to realign it now, and change it again later. Don't you think?
It is a peer dependency so it won't be bundled. Happy to refactor it and use css modules with less
There was a problem hiding this comment.
Styled components is one of the most popular solution now.
And yet, it's not the most popular one in the first userbase we are going after with this MIT version: https://deploy-preview-21555--material-ui.netlify.app/blog/2020-developer-survey-results/#19-what-styling-system-are-you-using
It is a peer dependency so it won't be bundled.
What happen if the peer dependency is missing? Does it crash?
If it doesn't crash, why do we need to include it? If it crashes, it will be bundled.
I think it is a waste of time to realign it now, and change it again later. Don't you think?
No strong preference, I'm curious to see how this will play out. I wanted to raise the concern for the record, so we can come back to it in 6 months and reevaluate.
There was a problem hiding this comment.
The main reason for adding it as peer dependency is to allow the consumer to control the version of the package and avoid having several versions of the same package in the components tree which usually have disastrous consequences if packages interact with each other like for react.
TBH I'm not a big fan of having any dependencies beside react, let see if we can improve that in the future ;)
There was a problem hiding this comment.
The main reason
Users can control the version of the dependency even if it's not a peer dependency. The reason has to be found somewhere else. It's the same as why react and react-dom should be peer dependencies: they have a singleton. SC explains a bit more why in https://styled-components.com/docs/faqs#i-am-a-library-author-should-i-bundle-styledcomponents-with-my-library and https://styled-components.com/docs/faqs#why-am-i-getting-a-warning-about-several-instances-of-module-on-the-page.
TBH I'm not a big fan of having any dependencies beside react, let see if we can improve that in the future ;)
💯 so much agree here, I hope we can take this problem down in v5, it will both help for users asking for better bundle size (no duplication), users asking for easier customization (no need to configure the CSS injection order with another tool), help with the marketing perceived value (you are in control, it's light).
| @@ -0,0 +1,6042 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
Something is wrong with the yarn.lock. They should be a single file at the root, right now, I could at least count 4 files.
| @@ -29,7 +29,7 @@ | |||
| "peerDependencies": { | |||
| "@material-ui/core": "^4.9.12", | |||
| "react": "^16.13.1", | |||
| "styled-components": "^5.0.1" | |||
| "styled-components": "^5.1.0" | |||
There was a problem hiding this comment.
Why do we need to increase the minimum version?
There was a problem hiding this comment.
official version is 5.1.0 across muix. I just reconciled
There was a problem hiding this comment.
I meant, why have 5.1.0 as the lower accepted version and not 5.0.0? Is there any feature or bug fixes we need? I wonder because the larger the peer dependency scope is, the better/simpler for end-users.
| const story = await getStoryPage('/story/x-grid-tests-columns--small-col-sizes'); | ||
| page = story.page; | ||
| browser = story.browser; | ||
| await page.keyboard.press('Tab'); |
There was a problem hiding this comment.
It sounds like we are introducing a third way to run the tests, with no clear advantages:
- In https://github.com/mui-org/material-ui: mocha + karma + testing-library + ChromeHeadless + JSDOM cc @eps1lon
- In https://github.com/mui-org/material-ui-pickers: Jest + Cypress cc @dmtrKovalenko
- In https://github.com/mui-org/material-ui-x: Jest + Puppeteer
@dtassone What do you think of reducing the number of tools we use on Material-UI and aiming for the test stack of the main repository? Similarly to mui/material-ui-pickers#1933, I believe we can get +90% of the value with the existing stack. If it's true, it means we can save time on the infrastructure, time we can invest in the components with a higher ROI for our users.
Sebastian is already making great progress on the infrastructure, it will be very hard to do anything better, I would be in favor of leveraging his work in the whole codebase (cross repositories).
Also, note that we try to move away from puppeteer to playwright in mui/material-ui#21449. For the visual regression test, I believe we should be using the exact same stack. The source components can come from storybook's stories, it's not an issue. I would simply copy and paste what we have in https://github.com/mui-org/material-ui or what we will have after mui/material-ui#21449.
There was a problem hiding this comment.
I personally would use playwright (instead of puppeteer) which is developed by former puppeteer maintainers and microsoft.
I don't share the concern of using different stacks. What's more important is that we share a common patterns for testing and all of these test frameworks/runners have integrations for testing-library
There was a problem hiding this comment.
puppeteer is maintained by the chrome team and is far more mature than playwright. 65k stars v4 vs 10k stars v1. On the other hand playwright support different browser whereas puppeteer is only chrome...
Api is very similar.
Cypress is a bigger tool, and it's more like a CI service...
mocha karma jsdom feels a bit outdated. Jest is faster, better integration and fb behind it.
There was a problem hiding this comment.
puppeteer is maintained by the chrome team
The devtools team open sourced it, yes. That team essentially moved to playwright (see top contributors).
is far more mature than playwright.
What makes you say that? You follow up with the stars count but that argument is invalid if you didn't control for project age (playwright was opensourced februrary 2020, puppeteer almost 3 years earlier)
On the other hand playwright support different browser whereas puppeteer is only chrome...
Api is very similar.
Puppeteer has experimental support for firefox.
mocha karma jsdom feels a bit outdated. Jest is faster, better integration and fb behind it.
Jest does use JSDOM and can only use JSDOM. It has no browser support (planned). "Faster" should be negligible. It comes mostly down to where the DOM implementation lives (JSDOM is slow, browser fast).
There was a problem hiding this comment.
I don't share the concern of using different stacks.
@eps1lon I think that we should take all opportunities to maximize synergies. Do we have any structural reason for having different testing stack between different Material-UI's components? Testing sounds like a support function, anything that works with "component A" should be able to work as well for "component B"? It seems better to compound around one approach.
@dtassone From what are I understand, these arguments are mainly driven by psychological marketing leverages (stars, outdated, fb, google, faster?), not by proven hard realities & pain points we face.
The main opposite argument seems to be, short term fragmentation is important so we can explore and discover better tools (and later converge). This sounds very much valid and something we should encourage. However, I think that it should start with the pain points. So I would propose we take a different angle to the problem:
What are the pain points with the testing stack on the main repository? How can these pain points be addresses with different tools? Does a new tool to the stack outweigh the cost of fragmentation (have to learn twice, improvements aren't shared)?
There was a problem hiding this comment.
I'm working on Material-UI, I can contribute to any component, I won't have to learn new APIs, debugging strategy, tools in order to do so.
This is subjective. Nobody can just drop in and contribute and expect that it works. If you're not willing to put the time in to make a robust contribution then that's fine. That's what maintainers are for. Otherwise you're describing unicorn developers.
I'm making an improvement to the stack, everybody in the team can benefit from it.
This holds true for a split stack? Obviously if you make change to unit test then e2e don't benefit. But there's a reason some tests are e2e and some unit.
you need to different effort to solve the same pain.
Exactly. You're making my argument. For different tests you use different stacks.
Or you want to use a single stack which means we can only do manual testing since that is the only method that allows testing every feature. In which case: Why bring it up now? Previous attempts to simplify the stack (drop test:karma) were rejected by you.
Your whole argument is based off the assumption that stacks are chosen arbitrarily. These choices are driven by technical arguments and therefore have technical implications. It would be more useful if you have concrete arguments why we shouldn't use puppeteer or cypress etc. These arguments can be made by showing how you could test the same approach with an existing stack. Then you should listen why the original author didn't make that decision.
There was a problem hiding this comment.
Well jest is faster, easier to configure, integrate with other tools and it's well maintained.
I believe those are facts and that's why the community is adopting it... Are you doing snapshot testing with Karma? How is the config...?
Some ref
https://codeburst.io/jest-the-next-generation-testing-8a6ee7c14656
https://www.npmtrends.com/jest-vs-karma-vs-mocha-vs-jasmine
There was a problem hiding this comment.
I went for puppeteer instead of playwright because
- I was happy only having chrome test for now
- Puppeteer seems more mature to me at this date than playwright.
- Api is pretty similar and both tool do pretty much the same thing so I don't think it's a big one.
But yes playwright is getting a lot of popularity :)
There was a problem hiding this comment.
This is subjective. Nobody can just drop in and contribute and expect that it works. If you're not willing to put the time in to make a robust contribution then that's fine. That's what maintainers are for. Otherwise, you're describing unicorn developers.
@eps1lon I'm making a reference to the dozens of developers we will hire in the coming months to grow and develop the components (MIT and paid, assuming enterprise works & we raise). My concern is with the internal full-time team, not external contributors: how can we make it as easy as possible for them.
Or you want to use a single stack which means we can only do manual testing since that is the only method that allows testing every feature. In which case: Why bring it up now? Previous attempts to simplify the stack (drop test:karma) were rejected by you.
I'm more than open to using different tools if there is a strong enough need for it. What I'm opposed to is using two different tools for solving the same problem. By same problem, I mean a solution that provides relatively the same set of features, e.g. more or less the same level of abstraction, more or less the same test duration, more or less the same features.
These arguments can be made by showing how you could test the same approach with an existing stack. Then you should listen why the original author didn't make that decision.
Yes! Let's do that! The whole assumption is that it duplicates. If it's true, we should use the test stack of the main repository. If it's false, we need to introduce new tradeoffs (new tools).
@dtassone what's the issue with the existing test stack that you wish to solve her?
snapshot testing
We are discouraging our users to have snapshot tests: mui/material-ui#21293, I think that we can remove this concern from the tradeoff.
puppeteer instead of playwright
I have no preference between the two, my point of interrogation was: Do we even need one of these two tools?
So far the conclusion seems to be: yes, for the more complex cases: for instance testing drag & drop, rendering performance, tabbing focus, screenshot (something we haven't done so far). And not for the common use cases, like a testing keyboard, clicks, focus, a11y (which should use mocha + karma + testing-library).
What's the best answer for the complex cases? I don't know, it seems we have 3 options: Cypress, Puppeteer, Playwright. How do we choose?
Regarding screenshot testing, I think that we should use whatever lands once mui/material-ui#21449 is resolved.
Is the above accurate?
There was a problem hiding this comment.
IMO if you build a new project, I would def not use karma jasmine, mocha, chai, sinon... I've use them before and jest made things much easier. Hence why a lot of ppl migrated.
Spies mocks assertions, are much easier to do with jest, much less dependencies.
We are discouraging our users to have snapshot tests: mui/material-ui#21293, I think that we can remove this concern from the tradeoff.
I disagree. Snapshot testing is easy to implement and provide a real value when it is done right. I can easily test all my storybook, and provide reliable regression tests.
It's not a solution for everything but it's great, it would be shame to throw it away because you generate a random value in one of your component.
oliviertassinari
added
the
component: data grid
No description provided.