-
Notifications
You must be signed in to change notification settings - Fork 147
Move rules settings to ESLint shared config: refactor prefer-presence-queries rule #252
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
Changes from all commits
aaee1b2
08d6d3b
95a3a9b
2037cda
e80e6bb
1c01e1d
a5034cc
9fba8ab
15e0a01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,19 @@ | ||
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { | ||
ASTUtils, | ||
TSESLint, | ||
TSESTree, | ||
} from '@typescript-eslint/experimental-utils'; | ||
import { | ||
getImportModuleName, | ||
getAssertNodeInfo, | ||
isLiteral, | ||
ImportModuleNode, | ||
isImportDeclaration, | ||
isImportNamespaceSpecifier, | ||
isImportSpecifier, | ||
isIdentifier, | ||
isProperty, | ||
} from './node-utils'; | ||
import { ABSENCE_MATCHERS, PRESENCE_MATCHERS } from './utils'; | ||
|
||
export type TestingLibrarySettings = { | ||
'testing-library/module'?: string; | ||
|
@@ -41,6 +46,11 @@ export type DetectionHelpers = { | |
getCustomModuleImportName: () => string | undefined; | ||
getIsTestingLibraryImported: () => boolean; | ||
getIsValidFilename: () => boolean; | ||
isGetByQuery: (node: TSESTree.Identifier) => boolean; | ||
isQueryByQuery: (node: TSESTree.Identifier) => boolean; | ||
isSyncQuery: (node: TSESTree.Identifier) => boolean; | ||
isPresenceAssert: (node: TSESTree.MemberExpression) => boolean; | ||
isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean; | ||
Comment on lines
+49
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I named these functions with prefix This implies I'll have to rename:
I'll do this in a separate PR tho. |
||
canReportErrors: () => boolean; | ||
findImportedUtilSpecifier: ( | ||
specifierName: string | ||
|
@@ -85,7 +95,8 @@ export function detectTestingLibraryUtils< | |
return getImportModuleName(importedCustomModuleNode); | ||
}, | ||
/** | ||
* Gets if Testing Library is considered as imported or not. | ||
* Determines whether Testing Library utils are imported or not for | ||
* current file being analyzed. | ||
* | ||
* By default, it is ALWAYS considered as imported. This is what we call | ||
* "aggressive reporting" so we don't miss TL utils reexported from | ||
|
@@ -105,17 +116,75 @@ export function detectTestingLibraryUtils< | |
}, | ||
|
||
/** | ||
* Gets if filename being analyzed is valid or not. | ||
* | ||
* This is based on "testing-library/filename-pattern" setting. | ||
* Determines whether filename is valid or not for current file | ||
* being analyzed based on "testing-library/filename-pattern" setting. | ||
*/ | ||
getIsValidFilename() { | ||
const fileName = context.getFilename(); | ||
return !!fileName.match(filenamePattern); | ||
}, | ||
|
||
/** | ||
* Wraps all conditions that must be met to report rules. | ||
* Determines whether a given node is `getBy*` or `getAllBy*` query variant or not. | ||
*/ | ||
isGetByQuery(node) { | ||
return !!node.name.match(/^get(All)?By.+$/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aggressive query reporting enabled! It doesn't matter if it's a Testing Library built-in query or a custom one: if it matches |
||
}, | ||
|
||
/** | ||
* Determines whether a given node is `queryBy*` or `queryAllBy*` query variant or not. | ||
*/ | ||
isQueryByQuery(node) { | ||
return !!node.name.match(/^query(All)?By.+$/); | ||
}, | ||
|
||
/** | ||
* Determines whether a given node is sync query or not. | ||
*/ | ||
isSyncQuery(node) { | ||
return this.isGetByQuery(node) || this.isQueryByQuery(node); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. I'll address that in a separate PR when open PRs are merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch 👍 |
||
}, | ||
|
||
/** | ||
* Determines whether a given MemberExpression node is a presence assert | ||
* | ||
* Presence asserts could have shape of: | ||
* - expect(element).toBeInTheDocument() | ||
* - expect(element).not.toBeNull() | ||
*/ | ||
isPresenceAssert(node) { | ||
const { matcher, isNegated } = getAssertNodeInfo(node); | ||
|
||
if (!matcher) { | ||
return false; | ||
} | ||
|
||
return isNegated | ||
? ABSENCE_MATCHERS.includes(matcher) | ||
: PRESENCE_MATCHERS.includes(matcher); | ||
}, | ||
|
||
/** | ||
* Determines whether a given MemberExpression node is an absence assert | ||
* | ||
* Absence asserts could have shape of: | ||
* - expect(element).toBeNull() | ||
* - expect(element).not.toBeInTheDocument() | ||
*/ | ||
isAbsenceAssert(node) { | ||
const { matcher, isNegated } = getAssertNodeInfo(node); | ||
|
||
if (!matcher) { | ||
return false; | ||
} | ||
|
||
return isNegated | ||
? PRESENCE_MATCHERS.includes(matcher) | ||
: ABSENCE_MATCHERS.includes(matcher); | ||
}, | ||
|
||
/** | ||
* Determines if file inspected meets all conditions to be reported by rules or not. | ||
*/ | ||
canReportErrors() { | ||
return ( | ||
|
@@ -144,7 +213,7 @@ export function detectTestingLibraryUtils< | |
return node.specifiers.find((n) => isImportNamespaceSpecifier(n)); | ||
} else { | ||
const requireNode = node.parent as TSESTree.VariableDeclarator; | ||
if (isIdentifier(requireNode.id)) { | ||
if (ASTUtils.isIdentifier(requireNode.id)) { | ||
// this is const rtl = require('foo') | ||
return requireNode.id; | ||
} | ||
|
@@ -153,7 +222,7 @@ export function detectTestingLibraryUtils< | |
const property = destructuring.properties.find( | ||
(n) => | ||
isProperty(n) && | ||
isIdentifier(n.key) && | ||
ASTUtils.isIdentifier(n.key) && | ||
n.key.name === specifierName | ||
); | ||
return (property as TSESTree.Property).key as TSESTree.Identifier; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { | ||
AST_NODE_TYPES, | ||
ASTUtils, | ||
TSESLint, | ||
TSESTree, | ||
} from '@typescript-eslint/experimental-utils'; | ||
|
@@ -253,3 +254,41 @@ export function getImportModuleName( | |
return node.arguments[0].value; | ||
} | ||
} | ||
|
||
type AssertNodeInfo = { | ||
matcher: string | null; | ||
isNegated: boolean; | ||
}; | ||
/** | ||
* Extracts matcher info from MemberExpression node representing an assert. | ||
*/ | ||
export function getAssertNodeInfo( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was extracted from the rule itself to get info about the assert node. Could be useful in other rules if assert statement impacts the rule reporting. It only takes |
||
node: TSESTree.MemberExpression | ||
): AssertNodeInfo { | ||
const emptyInfo = { matcher: null, isNegated: false } as AssertNodeInfo; | ||
|
||
if ( | ||
!isCallExpression(node.object) || | ||
!ASTUtils.isIdentifier(node.object.callee) | ||
) { | ||
return emptyInfo; | ||
} | ||
|
||
if (node.object.callee.name !== 'expect') { | ||
return emptyInfo; | ||
} | ||
|
||
let matcher = ASTUtils.getPropertyName(node); | ||
const isNegated = matcher === 'not'; | ||
if (isNegated) { | ||
matcher = isMemberExpression(node.parent) | ||
? ASTUtils.getPropertyName(node.parent) | ||
: null; | ||
} | ||
|
||
if (!matcher) { | ||
return emptyInfo; | ||
} | ||
|
||
return { matcher, isNegated }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,12 @@ | ||
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { | ||
getDocsUrl, | ||
ALL_QUERIES_METHODS, | ||
PRESENCE_MATCHERS, | ||
ABSENCE_MATCHERS, | ||
} from '../utils'; | ||
import { | ||
findClosestCallNode, | ||
isMemberExpression, | ||
isIdentifier, | ||
} from '../node-utils'; | ||
import { TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { findClosestCallNode, isMemberExpression } from '../node-utils'; | ||
import { createTestingLibraryRule } from '../create-testing-library-rule'; | ||
|
||
export const RULE_NAME = 'prefer-presence-queries'; | ||
export type MessageIds = 'presenceQuery' | 'absenceQuery' | 'expectQueryBy'; | ||
export type MessageIds = 'wrongPresenceQuery' | 'wrongAbsenceQuery'; | ||
type Options = []; | ||
|
||
const QUERIES_REGEXP = new RegExp( | ||
`^(get|query)(All)?(${ALL_QUERIES_METHODS.join('|')})$` | ||
); | ||
|
||
function isThrowingQuery(node: TSESTree.Identifier) { | ||
return node.name.startsWith('get'); | ||
} | ||
|
||
export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | ||
export default createTestingLibraryRule<Options, MessageIds>({ | ||
name: RULE_NAME, | ||
meta: { | ||
docs: { | ||
|
@@ -33,62 +16,47 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | |
recommended: 'error', | ||
}, | ||
messages: { | ||
presenceQuery: | ||
wrongPresenceQuery: | ||
'Use `getBy*` queries rather than `queryBy*` for checking element is present', | ||
absenceQuery: | ||
wrongAbsenceQuery: | ||
'Use `queryBy*` queries rather than `getBy*` for checking element is NOT present', | ||
expectQueryBy: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one wasn't used. |
||
'Use `getBy*` only when checking elements are present, otherwise use `queryBy*`', | ||
}, | ||
schema: [], | ||
type: 'suggestion', | ||
fixable: null, | ||
}, | ||
defaultOptions: [], | ||
|
||
create(context) { | ||
create(context, _, helpers) { | ||
return { | ||
[`CallExpression Identifier[name=${QUERIES_REGEXP}]`]( | ||
node: TSESTree.Identifier | ||
) { | ||
'CallExpression Identifier'(node: TSESTree.Identifier) { | ||
const expectCallNode = findClosestCallNode(node, 'expect'); | ||
|
||
if (expectCallNode && isMemberExpression(expectCallNode.parent)) { | ||
const expectStatement = expectCallNode.parent; | ||
const property = expectStatement.property as TSESTree.Identifier; | ||
let matcher = property.name; | ||
let isNegatedMatcher = false; | ||
if (!expectCallNode || !isMemberExpression(expectCallNode.parent)) { | ||
return; | ||
} | ||
|
||
if ( | ||
matcher === 'not' && | ||
isMemberExpression(expectStatement.parent) && | ||
isIdentifier(expectStatement.parent.property) | ||
) { | ||
isNegatedMatcher = true; | ||
matcher = expectStatement.parent.property.name; | ||
} | ||
// Sync queries (getBy and queryBy) are corresponding ones used | ||
// to check presence or absence. If none found, stop the rule. | ||
if (!helpers.isSyncQuery(node)) { | ||
return; | ||
} | ||
|
||
const validMatchers = isThrowingQuery(node) | ||
? PRESENCE_MATCHERS | ||
: ABSENCE_MATCHERS; | ||
const isPresenceQuery = helpers.isGetByQuery(node); | ||
const expectStatement = expectCallNode.parent; | ||
const isPresenceAssert = helpers.isPresenceAssert(expectStatement); | ||
const isAbsenceAssert = helpers.isAbsenceAssert(expectStatement); | ||
|
||
const invalidMatchers = isThrowingQuery(node) | ||
? ABSENCE_MATCHERS | ||
: PRESENCE_MATCHERS; | ||
if (!isPresenceAssert && !isAbsenceAssert) { | ||
return; | ||
} | ||
|
||
const messageId = isThrowingQuery(node) | ||
? 'absenceQuery' | ||
: 'presenceQuery'; | ||
if (isPresenceAssert && !isPresenceQuery) { | ||
return context.report({ node, messageId: 'wrongPresenceQuery' }); | ||
} | ||
|
||
if ( | ||
(!isNegatedMatcher && invalidMatchers.includes(matcher)) || | ||
(isNegatedMatcher && validMatchers.includes(matcher)) | ||
) { | ||
return context.report({ | ||
node, | ||
messageId, | ||
}); | ||
} | ||
if (isAbsenceAssert && isPresenceQuery) { | ||
return context.report({ node, messageId: 'wrongAbsenceQuery' }); | ||
} | ||
}, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ const LIBRARY_MODULES = [ | |
'@testing-library/svelte', | ||
]; | ||
|
||
// TODO: should be deleted after all rules are migrated to v4 | ||
const hasTestingLibraryImportModule = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method must disappear after refactoring all rules since this check it's part of detection module now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we mark it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will disappear when v4 is finished so I don't think we even need to deprecate it |
||
node: TSESTree.ImportDeclaration | ||
): boolean => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are setting max-warnings to 0 in the CI so I think it's better to have same conditions in precommit hook.