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

[core] Set up eslint-plugin-testing-library #42909

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Jul 11, 2024

Part of mui/mui-public#173. Sets up eslint-plugin-testing-library and fix related issues.

Two rules have been disabled given the amount of errors reported:

@aarongarciah aarongarciah added test scope: code-infra Specific to the core-infra product labels Jul 11, 2024
@aarongarciah aarongarciah changed the title [core-infra] Set up eslint-plugin-testing-library [code-infra] Set up eslint-plugin-testing-library Jul 11, 2024
@aarongarciah aarongarciah force-pushed the eslint-plugin-testing-library branch from 4df5802 to 1db11f8 Compare July 16, 2024 12:04
@mui-bot
Copy link

mui-bot commented Jul 16, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against d2bd8ba

@aarongarciah aarongarciah added the on hold There is a blocker, we need to wait label Jul 16, 2024
@aarongarciah aarongarciah force-pushed the eslint-plugin-testing-library branch 2 times, most recently from 52d240a to d46c445 Compare July 16, 2024 15:12
@aarongarciah aarongarciah changed the title [code-infra] Set up eslint-plugin-testing-library [core] Set up eslint-plugin-testing-library Jul 16, 2024
@aarongarciah aarongarciah added core Infrastructure work going on behind the scenes and removed scope: code-infra Specific to the core-infra product labels Jul 16, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024
package.json Show resolved Hide resolved
@Janpot
Copy link
Member

Janpot commented Jul 18, 2024

Not fully reviewing yet, but to note that this PR will need to be canary tested in dependent projects before merging

@aarongarciah aarongarciah removed the on hold There is a blocker, we need to wait label Jul 18, 2024
@aarongarciah aarongarciah force-pushed the eslint-plugin-testing-library branch from d46c445 to b9c14fc Compare July 18, 2024 08:04
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024
@aarongarciah aarongarciah force-pushed the eslint-plugin-testing-library branch from b9c14fc to 1fd0dec Compare July 18, 2024 08:09
@@ -685,6 +689,8 @@ describe('<Tabs />', () => {

await waitFor(() => {
expect(hasLeftScrollButton(container)).to.equal(true);
});
await waitFor(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@aarongarciah aarongarciah requested a review from a team July 18, 2024 08:12
@aarongarciah
Copy link
Member Author

aarongarciah commented Jul 18, 2024

This is now ready for review. Keeping it in draft so it's clearer it's not ready to be merged until it's canary tested.

Comment on lines +279 to +293
{
files: [
// matching the pattern of the test runner
'*.test.mjs',
'*.test.js',
'*.test.ts',
'*.test.tsx',
],
excludedFiles: ['packages/markdown/**/*', 'test/e2e/**/*', 'test/regressions/**/*'],
extends: ['plugin:testing-library/react'],
rules: {
'testing-library/no-container': 'off',
'testing-library/prefer-screen-queries': 'off',
},
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Configuring the ESLint plugin using overrides so it only runs on test files. See docs for more info https://github.com/testing-library/eslint-plugin-testing-library?tab=readme-ov-file#eslint-overrides

@@ -25,6 +25,7 @@ describe('<ClickAwayListener />', () => {
* React bug: https://github.com/facebook/react/issues/20074
*/
function render(...args) {
// eslint-disable-next-line testing-library/render-result-naming-convention
Copy link
Member Author

Choose a reason for hiding this comment

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

The render-result-naming-convention rule seems to warn whenever it finds a function/method name that contains "render" 😅

Copy link
Member

@Janpot Janpot Jul 18, 2024

Choose a reason for hiding this comment

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

oh 🤦 . Maybe we should just disable this one rule?

@@ -83,6 +83,7 @@ describe('mochaHooks', () => {
// not wrapped in act()
unsafeSetState(1);
// make sure effects are flushed
// eslint-disable-next-line testing-library/no-unnecessary-act
act(() => {});
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be async as well?

Suggested change
act(() => {});
await act(async () => {});

Perhaps it could make sense to create a await flushEffects() helper?

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially started to await all act but realized there were too many changes. I decided to await all act calls in another PR, but if you think it makes sense to make it all at once I'm ok with it, too.

Copy link
Member

Choose a reason for hiding this comment

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

No, it would be good to do separate as far as I'm concerned.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it could make sense to create a await flushEffects() helper?

There is one: flushMicrotasks in internal-test-utils

act(() => {
button.focus();
});
button.focus();
Copy link
Member

Choose a reason for hiding this comment

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

Would expect this one to be wrapped in act.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! True, not sure why/how I did this. Fixed.

@@ -25,6 +25,7 @@ describe('<ClickAwayListener />', () => {
* React bug: https://github.com/facebook/react/issues/20074
*/
function render(...args) {
// eslint-disable-next-line testing-library/render-result-naming-convention
const result = clientRender(...args);

This comment was marked as outdated.

@aarongarciah
Copy link
Member Author

After chatting with @Janpot, I'll create a separate PR with test changes and leave this one only with the ESLint plugin setup.

@@ -111,9 +111,7 @@ describe('<AccordionSummary />', () => {

// this doesn't actually apply focus like in the browser. we need to move focus manually
Copy link
Member

Choose a reason for hiding this comment

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

... or use @testing-library/user-event 🙂

@aarongarciah aarongarciah force-pushed the eslint-plugin-testing-library branch from 1fd0dec to ea057c7 Compare July 18, 2024 08:56
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants