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.

Sign up for GitHub

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

Jump to bottom

fix: avoid reporting react-test-renderer utils as queries #709

Closed
wants to merge 2 commits into from
Closed

fix: avoid reporting react-test-renderer utils as queries #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92861eb
fix: avoid reporting react-test-renderer utils as find queries
Belco90 Dec 23, 2022
fc3df1c
fix: avoid reporting react-test-renderer utils as custom queries
Belco90 Dec 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions lib/create-testing-library-rule/detect-testing-library-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,14 @@ const FIRE_EVENT_NAME = 'fireEvent';
const CREATE_EVENT_NAME = 'createEvent';
const USER_EVENT_NAME = 'userEvent';
const RENDER_NAME = 'render';
const EXCLUDED_FIND_BY_QUERIES = [
//react-test-renderer utils
'findByType',
'findByProps',
'findAll',
'findAllByType',
'findAllByProps',
Comment on lines +138 to +142
Copy link

Choose a reason for hiding this comment

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

And what if someone uses a custom query with one of these names? We still want them to be reported. no? How can we make sure that we are dealing with a react-test-renderer query?
This relates also to this comment.

Copy link
Member Author

@Belco90 Belco90 Dec 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That would be the ideal fix indeed. However, since the utils names seem quite specific and can be chained from built-in queries in React Native, this PR could be a quick workaround.

I personally don't have the time to implement the ideal fix to check if queries are coming from react-test-renderer, so it's a matter of deciding if we are happy with this workaround (and its trade-offs) or we are only interested in the ideal fix.

Copy link
Member Author

@Belco90 Belco90 Dec 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Another idea could be adding a config for React Native to the plugin, which adds automatically the corresponding query names to the settings to be ignored as custom queries.

This would be a workaround too, but perhaps makes more sense since it would affect just users using the RN preset.

];

export type DetectionOptions = {
/**
Expand Down Expand Up @@ -389,7 +397,11 @@ export function detectTestingLibraryUtils<
* Determines whether a given node is `find*` query variant or not.
*/
const isFindQueryVariant: IsFindQueryVariantFn = (node) => {
return isQuery(node) && node.name.startsWith('find');
return (
isQuery(node) &&
node.name.startsWith('find') &&
!EXCLUDED_FIND_BY_QUERIES.includes(node.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.

Actually... I could add this to isQuery instead.

);
};

/**
Expand All @@ -407,7 +419,11 @@ export function detectTestingLibraryUtils<
};

const isCustomQuery: IsCustomQueryFn = (node) => {
return isQuery(node) && !ALL_QUERIES_COMBINATIONS.includes(node.name);
return (
isQuery(node) &&
!ALL_QUERIES_COMBINATIONS.includes(node.name) &&
!EXCLUDED_FIND_BY_QUERIES.includes(node.name)
);
};

const isBuiltInQuery = (node: TSESTree.Identifier): boolean => {
Expand Down
10 changes: 10 additions & 0 deletions tests/create-testing-library-rule.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,16 @@ ruleTester.run(RULE_NAME, rule, {
`,
},

// `react-test-renderer` utils starting with "find" shouldn't be reported as queries
// https://github.com/testing-library/eslint-plugin-testing-library/issues/673
`// issue #673
findByType('foo');
findByProps('foo');
findAll('foo');
findAllByType('foo');
findAllByProps('foo');
`,

// Weird edge cases
`(window as any).__THING = false;`,
`thing.method.lastCall.args[0]();`,
Expand Down
6 changes: 6 additions & 0 deletions tests/lib/rules/await-async-query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,12 @@ ruleTester.run(RULE_NAME, rule, {
})
`,

// https://github.com/testing-library/eslint-plugin-testing-library/issues/671
`// issue #671

const elements = root.findAllByType('div')
`,

// edge case for coverage
// valid async query usage without any function defined
// so there is no innermost function scope found
Expand Down