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

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

Merged
merged 18 commits into from
Aug 20, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Aug 16, 2024

Related to mui/material-ui#42909.

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

@LukasTy LukasTy added test core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product labels Aug 16, 2024
@LukasTy LukasTy self-assigned this Aug 16, 2024
@mui-bot
Copy link

mui-bot commented Aug 16, 2024

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

Generated by 🚫 dangerJS against c90c2e6

});
expect(getCell(6, 2).textContent).to.equal('p62');
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -24,7 +24,7 @@ import { describeConformance } from 'test/utils/describeConformance';
import { RangePosition } from '../models';

const getPickerDay = (name: string, picker = 'January 2018') =>
getByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });
rtlGetByRole(screen.getByRole('grid', { name: picker }), 'gridcell', { name });
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps use within instead?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -13,31 +13,31 @@ describe('<SingleInputDateRangeField /> - Editing', () => {
describeAdapters(`key: Delete`, SingleInputDateRangeField, ({ adapter, renderWithProps }) => {
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({
Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🙈

getData: () => pastedValue,
};
let canContinue = true;
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.

Only the dispatching of an event needed the act wrapping.

Signed-off-by: Lukas Tyla <llukas.tyla@gmail.com>
@LukasTy LukasTy requested review from a team August 16, 2024 11:32
@@ -409,8 +409,8 @@ describe('<DataGridPremium /> - Aggregation', () => {

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

Choose a reason for hiding this comment

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

Turning these async can also be done in follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Janpot Janpot Aug 16, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@LukasTy LukasTy Aug 20, 2024

Choose a reason for hiding this comment

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

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

Comment on lines 1 to 26
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,
};
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🙈

package.json Outdated Show resolved Hide resolved
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

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?

@@ -24,7 +24,7 @@ import { describeConformance } from 'test/utils/describeConformance';
import { RangePosition } from '../models';

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

Choose a reason for hiding this comment

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

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 LukasTy merged commit a943aef into mui:master Aug 20, 2024
18 checks passed
@LukasTy LukasTy deleted the setup-eslint-plugin-testing-library branch August 20, 2024 10:32
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 scope: code-infra Specific to the core-infra product test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants