[core] Support @mui/material@6 peer dependency
#14142
Conversation
|
Deploy preview: https://deploy-preview-14142--material-ui-x.netlify.app/ |
| import packageJson from '@mui/material/package.json'; | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log(`@mui/x-charts-pro: @mui/material version`, packageJson.version); |
There was a problem hiding this comment.
This is to make sure that tests are actually using @mui/material@^6:
packages/x-charts-pro/package.json
Outdated
| @@ -66,6 +66,7 @@ | |||
| } | |||
| }, | |||
| "devDependencies": { | |||
| "@mui/material": "next", | |||
There was a problem hiding this comment.
The dev dependency here is the key to running the tests against the specific version of @mui/material.
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
added
the
PR: out-of-date
| const onAccept = spy(); | ||
| const onClear = spy(); | ||
| const onCancel = spy(); | ||
| const onSetToday = spy(); | ||
|
|
||
| render( | ||
| const { user } = render( |
There was a problem hiding this comment.
This looks off the direction set in: mui/mui-public#173
| const { user } = render( | |
| const render( |
There was a problem hiding this comment.
The user from render comes from userEvent.setup(), we should probably validate if there's something important done in that setup call:
There was a problem hiding this comment.
@oliviertassinari @testing-library/react doesn't have a user export. They advise to initialize a new instance for every test which is what we do in our render method wrapper.
Personally, I find overly abstracting things in tests leads to harder to maintain code. Wrappers tend to want to grow indefinitely as the project progresses. But since in core the wrapper was there and used everywhere, it guess it made sense to fold the userEvent code into it. For tests I believe the DRY principle sometimes is harmful. People end up writing a bunch of abstractions over their testing code and they end up becoming just as hard to refactor as the main code.
There was a problem hiding this comment.
we should probably validate if there's something important done in that setup call:
If necessary, you can chain multiple setup calls with different options, see https://github.com/mui/mui-x/pull/14142/files#diff-c307c02f110006557e5c75d52b6f6eef030b543ba4b193c936cce9909902f930R113-R115
There was a problem hiding this comment.
Note that, while directly invoking APIs such as
userEvent.click()(which will trigger setup internally) is still supported in v14, this option exists to ease the migration from v13 to v14, and for simple tests. We recommend using the methods on the instances returned byuserEvent.setup().
The way I understand it is that the instance holds some state around the "user session". i.e. It makes sense to share a single instance per test to maintain that state between calls. But to create a new instance per test to reset the state across different tests, and perhaps also to avoid interference between parallel tests?
There was a problem hiding this comment.
We should open a new issue to discuss improving the current user setup. It is out of topic here.
There was a problem hiding this comment.
we should probably validate if there's something important done in that setup call:
@romgrk Setup doesn't seem to do a lot https://github.com/testing-library/user-event/blob/d0362796a33c2d39713998f82ae309020c37b385/src/setup/setup.ts#L82. I was worried about the performance slowness of running in each test, but it doesn't seem that bad. Not speeding the CI though 😄.
They advise to initialize a new instance for every test which is what we do in our render method wrapper.
@Janpot In the why they advice against it, they seems to point against using beforeEach, but it's not what we would do here?
We should open a new issue to discuss improving the current user setup. It is out of topic here.
I think we can use mui/mui-public#173 for this.
There was a problem hiding this comment.
We recommend invoking userEvent.setup() before the component is rendered. This can be done in the test itself, or by using a setup function. We discourage rendering or using any userEvent functions outside of the test itself - e.g. in a before/after hook - for reasons described in "Avoid Nesting When You're Testing".
afaiu, what we do falls under "by using a setup function".
Fixes #14055.
Fixes #14369.
TODO:
@mui/materialversion check@mui/material@next@mui/material@5@mui/materialversion tonexttest_typesjob tomaterial-ui-v6workflowmaterial-ui-v6workflow passmaterial-ui-v6workflow?material-ui-v6workflow? Is it possible to have 2 Argos checks on the PR?Edit: the default workflow sends screenshots to Argos. The
material-ui-v6workflow also sends screenshots to Argos effectively overriding the ones from the default workflow. I think it's OK.