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

aarongarciah · 2024-07-11T11:32:14Z

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:

prefer-screen-queries: 2146 errors
no-container: 576 errors

mui-bot · 2024-07-16T12:07:46Z

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

package.json

Janpot · 2024-07-18T08:00:03Z

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

aarongarciah · 2024-07-18T08:10:52Z

packages/mui-material/src/Tabs/Tabs.test.js

@@ -685,6 +689,8 @@ describe('<Tabs />', () => {

        await waitFor(() => {
          expect(hasLeftScrollButton(container)).to.equal(true);
+        });
+        await waitFor(() => {


Only one expect per waitFor. Related rule https://github.com/testing-library/eslint-plugin-testing-library/blob/main/docs/rules/no-wait-for-multiple-assertions.md

aarongarciah · 2024-07-18T08:13:23Z

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.

aarongarciah · 2024-07-18T08:17:42Z

.eslintrc.js

+    {
+      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',
+      },
+    },


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

aarongarciah · 2024-07-18T08:19:27Z

packages/mui-base/src/ClickAwayListener/ClickAwayListener.test.js

@@ -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


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

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

Janpot · 2024-07-18T08:22:32Z

packages-internal/test-utils/src/mochaHooks.test.js

@@ -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(() => {});


Shouldn't this be async as well?

Suggested change

act(() => {});

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

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

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.

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

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

There is one: flushMicrotasks in internal-test-utils

Janpot · 2024-07-18T08:23:58Z

packages/mui-base/src/Button/Button.test.tsx

-        act(() => {
-          button.focus();
-        });
+        button.focus();


Would expect this one to be wrapped in act.

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

packages/mui-base/src/ClickAwayListener/ClickAwayListener.test.js

@@ -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);


aarongarciah · 2024-07-18T08:27:05Z

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

Janpot · 2024-07-18T08:29:26Z

packages/mui-material/src/AccordionSummary/AccordionSummary.test.js

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

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


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

aarongarciah added test scope: code-infra Specific to the core-infra product labels Jul 11, 2024

aarongarciah changed the title ~~[core-infra] Set up eslint-plugin-testing-library~~ [code-infra] Set up eslint-plugin-testing-library Jul 11, 2024

aarongarciah mentioned this pull request Jul 12, 2024

[material-ui] Performance: lazy Ripple #41061

Merged

aarongarciah force-pushed the eslint-plugin-testing-library branch from 4df5802 to 1db11f8 Compare July 16, 2024 12:04

aarongarciah added the on hold There is a blocker, we need to wait label Jul 16, 2024

aarongarciah force-pushed the eslint-plugin-testing-library branch 2 times, most recently from 52d240a to d46c445 Compare July 16, 2024 15:12

aarongarciah changed the title ~~[code-infra] Set up eslint-plugin-testing-library~~ [core] Set up eslint-plugin-testing-library Jul 16, 2024

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

Set up eslint-plugin-testing-library

36c6b28

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024

Janpot reviewed Jul 18, 2024

View reviewed changes

package.json Show resolved Hide resolved

aarongarciah removed the on hold There is a blocker, we need to wait label Jul 18, 2024

aarongarciah force-pushed the eslint-plugin-testing-library branch from d46c445 to b9c14fc Compare July 18, 2024 08:04

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 force-pushed the eslint-plugin-testing-library branch from b9c14fc to 1fd0dec Compare July 18, 2024 08:09

aarongarciah commented Jul 18, 2024

View reviewed changes

aarongarciah requested a review from a team July 18, 2024 08:12

aarongarciah commented Jul 18, 2024

View reviewed changes

Janpot reviewed Jul 18, 2024

View reviewed changes

Fix errors

ea057c7

aarongarciah force-pushed the eslint-plugin-testing-library branch from 1fd0dec to ea057c7 Compare July 18, 2024 08:56

Wrap .focus() in act

d2bd8ba

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024

Janpot assigned aarongarciah Aug 9, 2024

LukasTy mentioned this pull request Aug 16, 2024

[code-infra] Set up eslint-plugin-testing-library mui/mui-x#14232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aarongarciah commented Jul 11, 2024 •

edited

Loading

mui-bot commented Jul 16, 2024 •

edited

Loading

Janpot commented Jul 18, 2024 •

edited

Loading

aarongarciah Jul 18, 2024

aarongarciah commented Jul 18, 2024 •

edited

Loading

aarongarciah Jul 18, 2024

aarongarciah Jul 18, 2024

Janpot Jul 18, 2024 •

edited

Loading

Janpot Jul 18, 2024

aarongarciah Jul 18, 2024

Janpot Jul 18, 2024

michaldudak Aug 6, 2024

Janpot Jul 18, 2024

aarongarciah Jul 18, 2024

This comment was marked as outdated.

aarongarciah commented Jul 18, 2024

Janpot Jul 18, 2024

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

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

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

Are you sure you want to change the base?

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

Conversation

aarongarciah commented Jul 11, 2024 • edited Loading

mui-bot commented Jul 16, 2024 • edited Loading

Netlify deploy preview

Bundle size report

Janpot commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

aarongarciah commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

aarongarciah commented Jul 18, 2024

Choose a reason for hiding this comment

aarongarciah commented Jul 11, 2024 •

edited

Loading

mui-bot commented Jul 16, 2024 •

edited

Loading

Janpot commented Jul 18, 2024 •

edited

Loading

aarongarciah commented Jul 18, 2024 •

edited

Loading

Janpot Jul 18, 2024 •

edited

Loading