Skip to content

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

Merged
merged 9 commits into from
Nov 10, 2020
2 changes: 1 addition & 1 deletion .lintstagedrc
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"*.{js,ts}": [
"eslint --fix",
"eslint --max-warnings 0 --fix",
Copy link
Member Author

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.

"prettier --write",
"jest --findRelatedTests"
],
Expand Down
87 changes: 78 additions & 9 deletions lib/detect-testing-library-utils.ts
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;
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

I named these functions with prefix is. I prefer to use prefix is, has, can for vars, and getIs, getHas , getCan for methods returning booleans. However, @typescript-eslint/experimental-utils is using the former for all its utils so I think to follow same standard ours should be named in the same way.

This implies I'll have to rename:

  • getIsTestingLibraryImported to isTestingLibraryImported
  • getIsValidFilename to isValidFilename

I'll do this in a separate PR tho.

canReportErrors: () => boolean;
findImportedUtilSpecifier: (
specifierName: string
Expand Down Expand Up @@ -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
Expand All @@ -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.+$/);
Copy link
Member Author

Choose a reason for hiding this comment

The 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 getBy* query pattern, it's considered like so. Same for queryBy* queries, and findBy* in the future.

},

/**
* 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps we should use helpers instead of this to avoid losing the type inference (as discussed in discord)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 (
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down
39 changes: 39 additions & 0 deletions lib/node-utils.ts
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';
Expand Down Expand Up @@ -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(
Copy link
Member Author

@Belco90 Belco90 Nov 5, 2020

Choose a reason for hiding this comment

The 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 jest asserts into account for now tho.

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 };
}
90 changes: 29 additions & 61 deletions lib/rules/prefer-presence-queries.ts
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: {
Expand All @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

The 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' });
}
},
};
Expand Down
1 change: 1 addition & 0 deletions lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const LIBRARY_MODULES = [
'@testing-library/svelte',
];

// TODO: should be deleted after all rules are migrated to v4
const hasTestingLibraryImportModule = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Should we mark it as @deprecated, because you'll get a warning if you want to use it?
Could be helpful for contributors.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 => {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"scripts": {
"build": "tsc",
"postbuild": "cpy README.md ./dist && cpy package.json ./dist && cpy LICENSE ./dist",
"lint": "eslint . --ext .js,.ts",
"lint": "eslint . --max-warnings 0 --ext .js,.ts",
"lint:fix": "npm run lint -- --fix",
"format": "prettier --write README.md \"{lib,docs,tests}/**/*.{js,ts,md}\"",
"format:check": "prettier --check README.md \"{lib,docs,tests}/**/*.{js,json,yml,ts,md}\"",
Expand Down
Loading