-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[code-infra] Enable testing-library eslint rules #47074
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
Conversation
Netlify deploy previewhttps://deploy-preview-47074--material-ui.netlify.app/ Bundle size report
|
5862b15 to
070a9bb
Compare
eslint.config.mjs
Outdated
| // Test start | ||
| { | ||
| files: [`**/*${EXTENSION_TEST_FILE}`, 'packages/mui-codemod/testUtils/**/*'], | ||
| files: [`**/*${EXTENSION_TEST_FILE}`, 'packages/mui-codemod/testUtils/index.js'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can maintain the broad application, in case someone decides to refactor in multiple files?
| files: [`**/*${EXTENSION_TEST_FILE}`, 'packages/mui-codemod/testUtils/index.js'], | |
| files: [`**/*${EXTENSION_TEST_FILE}`, `packages/mui-codemod/testUtils/**/*${EXTENSION_TS}`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this file from here altogether. Added the exception in the file itself.
| it('when undefined onChange and controlled should not call the onChange', () => { | ||
| const handleChange = spy(); | ||
| const { setProps, getByText } = render( | ||
| const { setProps } = render( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about discouraging destructuring here and instead
| const { setProps } = render( | |
| const view = render( |
Last time I worked on this code it was harder than it should be because this destructuring is hard to search on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of search are you referring to?
Also, not sure how we'd enforce not to destructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of search are you referring to?
It's convenient to search for things like "view.setProps(".
Also, not sure how we'd enforce not to destructure.
I thought there was a eslint rule in rtl plugin, but seems like i was wrong. Let's not worry about it.
Janpot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few remarks, but nothing blocking
c398975 to
006e9db
Compare
and fixes accordingly
Part of - mui/mui-public#173