[code-infra] Set up `eslint-plugin-testing-library` by LukasTy · Pull Request #14232 · mui/mui-x

LukasTy · 2024-08-16T10:36:55Z

~~It needs changes from mui/material-ui#43313 because the plugin treats userEvent.<function> calls as ones from @testing-library/user-event and tries awaiting them when it is not necessary.~~
Decided to remove the exported userEvent from @mui/internal-test-utils and move it only to mui-x as no other package is using it. 🙈
The existing userEvent naming is also confusing given that it is the same as the mentioned library, which we also now use.

Disable/turn off the no-container rule to allow doing container.querySelector
Rename response from render functions to view as only it and utils are allowed as per the plugin rule

mui-bot · 2024-08-16T10:41:02Z

Deploy preview: https://deploy-preview-14232--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c90c2e6

LukasTy · 2024-08-16T10:46:35Z

-          expect(getCell(7, 1).textContent).to.equal('p71');
-          expect(getCell(7, 3).textContent).to.equal('p73');
        });
+        expect(getCell(6, 2).textContent).to.equal('p62');


waitFor allows only one expect to be inside of it and it makes sense—anything after it can be outside of the awaited waitFor. 👌

LukasTy · 2024-08-16T10:50:36Z


 const getPickerDay = (name: string, picker = 'January 2018') =>
-  getByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });
+  rtlGetByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });


Using an aliased method avoids triggering the ESLint error and possibly improves readability. 🤔

rtl confused me at first, because I thought "Why would we need a different getByRole for Right-to-Left"? 😅
But since rtl stands for React Testing Library, isn't the naming still misleading given that we don't import it from the RTL package?

On a different note, why avoiding ESLint errors here?

On a different note, why avoiding ESLint errors here?

Would you suggest adding an ESLint ignore comment?
In this case, we want to use a root level getter and provide a specific container for it.
In other words, we are knowingly using the root getter instead of the one provided by screen.

rtl confused me at first, because I thought "Why would we need a different getByRole for Right-to-Left"? 😅
But since rtl stands for React Testing Library, isn't the naming still misleading given that we don't import it from the RTL package?

Agreed, I didn't think of rtl like that initially. 🙈
Will alias it as rootGetByRole instead. 👍

Perhaps use within instead?

- rtlGetByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name }); + within(screen.getByRole('grid', { name: picker })).getByRole('gridcell', { name });

@cherniavskii Thanks for the reminder, I completely forgot about within. 👍 🙈 🤷

LukasTy · 2024-08-16T10:54:58Z

    it('should clear all the sections when all sections are selected and all sections are completed', () => {
      // Test with v7 input
-      const v7Response = renderWithProps({
+      let view = renderWithProps({


Avoids the following error:

`v7Response` is not a recommended name for `render` returned value. Instead, you should destructure it, or name it using one of: `view`, or `utils`.

eslint(testing-library/render-result-naming-convention)

Destructuring in these cases seemed excessive as we need a lot of methods. 🙈

LukasTy · 2024-08-16T10:56:34Z

+        getData: () => pastedValue,
+      };
+      let canContinue = true;
+      act(() => {


Only the dispatching of an event needed the act wrapping.

Signed-off-by: Lukas Tyla <llukas.tyla@gmail.com>

Janpot · 2024-08-16T11:47:57Z

@@ -409,8 +409,8 @@ describe('<DataGridPremium /> - Aggregation', () => {

      act(() => apiRef.current.showColumnMenu('id'));


Turning these async can also be done in follow up.

Do you mean adding an await before act?
The plugin didn't complain about this. 🙈 🤷

Do you mean adding an await before act?

also making the function async

The plugin didn't complain about this. 🙈 🤷

...yet 😄 testing-library/eslint-plugin-testing-library#915

More info in this issue

Yeah... Well, it looks like it would be great to get the rule and then update the tests with that rule in action. 🙈

Janpot · 2024-08-16T15:02:14Z

If that's still the intention, I'd add a @deprecated comment to those exports with a note to use @testing-library/user-event instead.
perhaps with todo comment as well to update the repo to use @testing-library/user-event

I have deprecated the exports.
I had to go with an index re-export to keep the imports as is and reliably deprecate the exports. 👍
83e9b65

JCQuintas · 2024-08-19T14:58:27Z

+import { fireEvent } from '@mui/internal-test-utils/createRenderer';
+
+function touch(target: Element): void {
+  fireEvent.touchStart(target);
+  fireEvent.touchEnd(target);
+}
+
+const mousePress: (...args: Parameters<(typeof fireEvent)['mouseUp']>) => void = (
+  target,
+  options,
+) => {
+  fireEvent.mouseDown(target, options);
+  fireEvent.mouseUp(target, options);
+  fireEvent.click(target, options);
+};
+
+function keyPress(target: Element, options: { key: string; [key: string]: any }): void {
+  fireEvent.keyDown(target, options);
+  fireEvent.keyUp(target, options);
+}
+
+export const fireUserEvent = {
+  touch,
+  mousePress,
+  keyPress,
+};


Can't we use https://testing-library.com/docs/user-event/convenience?

Otherwise I would rename these to something that is less confusing, like fireCustomEvent or something similar.

Can't we use https://testing-library.com/docs/user-event/convenience?

That would be the ultimate goal, but after preliminary testing—it's not plug&play, we need more time and effort to migrate tests to using those APIs.

Otherwise I would rename these to something that is less confusing, like fireCustomEvent or something similar.

I have deprecated the exports to improve the clarity.
I don't think renaming it gives much value given that we'd want to remove those imports for the mentioned convenience API anyway eventually. 🙈

cherniavskii

Thanks for working on this!
I tried upgrading @mui/internal-test-utils in #14142, but then realized that the userEvent is no longer exported 😅 Should we upgrade it in this PR?

cherniavskii · 2024-08-19T20:00:44Z


 const getPickerDay = (name: string, picker = 'January 2018') =>
-  getByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });
+  rtlGetByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });


rtl confused me at first, because I thought "Why would we need a different getByRole for Right-to-Left"? 😅
But since rtl stands for React Testing Library, isn't the naming still misleading given that we don't import it from the RTL package?

On a different note, why avoiding ESLint errors here?

LukasTy added 8 commits August 15, 2024 10:58

Install plugin

dc7c01e

setup plugin

57d8952

Address plugin errors

38e937b

Ignore no-container error

aab1ee5

Use test-utils with renamed fireUserEvent export

7190437

Rename userEvent usages to fireUserEvent

b92ffad

Remove unnecessary act

a233de5

Wrap paste event dispatch in act

7012133

LukasTy added test internal Behind-the-scenes enhancement. Formerly called “core”. scope: code-infra labels Aug 16, 2024

LukasTy self-assigned this Aug 16, 2024

LukasTy commented Aug 16, 2024

View reviewed changes

Apply suggestions from code review

8be38a6

Signed-off-by: Lukas Tyla <llukas.tyla@gmail.com>

LukasTy requested review from a team August 16, 2024 11:32

Janpot reviewed Aug 16, 2024

View reviewed changes

LukasTy added 3 commits August 16, 2024 17:39

Use internal fireUserEvent

791044b

Install package with removed userEvent export to test

9ef5ba7

Revert @mui/internal-test-utils install

6cad254

Janpot reviewed Aug 16, 2024

View reviewed changes

Janpot mentioned this pull request Aug 19, 2024

[internal] Support @mui/material@6 peer dependency #14142

Merged

8 tasks

JCQuintas reviewed Aug 19, 2024

View reviewed changes

cherniavskii approved these changes Aug 19, 2024

View reviewed changes

cherniavskii and others added 4 commits August 19, 2024 22:37

Merge branch 'master' into setup-eslint-plugin-testing-library

71bf3bf

fix merge

63e364a

Use root alias for directly imported queries

c99765e

Deprecate fireUserEvent exports

83e9b65

LukasTy added 2 commits August 20, 2024 10:59

Simplify assertion

973a534

Use within instead of root getters

c90c2e6

LukasTy merged commit a943aef into mui:master Aug 20, 2024

LukasTy deleted the setup-eslint-plugin-testing-library branch August 20, 2024 10:32

alhabib5565 mentioned this pull request Sep 14, 2024

[Snyk] Upgrade: , , , , axios, dayjs, framer-motion, next, react-hook-form, swiper alhabib5565/quick-shop-client#15

Open

mbreadmachine mentioned this pull request Sep 14, 2024

[Snyk] Upgrade: , , , mbreadmachine/arany-d-osztaly-weboldal#33

Closed

KOTTAGENVH mentioned this pull request Sep 15, 2024

[Snyk] Upgrade: , , , , , , , , axios, chart.js, material-react-table, next KOTTAGENVH/converter-exchange#2

Open

alhabib5565 mentioned this pull request Sep 18, 2024

[Snyk] Upgrade: , , , , axios, dayjs, framer-motion, next, react-hook-form, swiper alhabib5565/quick-shop-client#16

Open

KOTTAGENVH mentioned this pull request Sep 18, 2024

[Snyk] Upgrade: , , , , , , , , axios, chart.js, material-react-table, next KOTTAGENVH/converter-exchange#3

Open

This was referenced Sep 19, 2024

[Snyk] Upgrade: , , , mbreadmachine/arany-d-osztaly-weboldal#34

Closed

[Snyk] Upgrade: , , , mbreadmachine/arany-d-osztaly-weboldal#35

Closed

mbreadmachine mentioned this pull request Sep 25, 2024

[Snyk] Upgrade @mui/x-date-pickers from 7.13.0 to 7.15.0 mbreadmachine/arany-d-osztaly-weboldal#37

Closed

		@@ -409,8 +409,8 @@ describe('<DataGridPremium /> - Aggregation', () => {

		act(() => apiRef.current.showColumnMenu('id'));

Uh oh!

Conversation

LukasTy commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Janpot Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasTy Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cherniavskii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LukasTy commented Aug 16, 2024 •

edited

Loading

mui-bot commented Aug 16, 2024 •

edited

Loading

Janpot Aug 16, 2024 •

edited

Loading

LukasTy Aug 20, 2024 •

edited

Loading