From 8a9973f0c3f4443c44eb6fbe73a2bc69d0b86309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 5 Nov 2020 11:14:29 +0100 Subject: [PATCH 1/4] refactor(extract helpers for detecting presence/absence assets): add fake rule tests for queries --- lib/detect-testing-library-utils.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index c3bf5401..a6faee9d 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -145,6 +145,30 @@ export function detectTestingLibraryUtils< return this.isGetByQuery(node) || this.isQueryByQuery(node); }, + isPresenceAssert(node) { + const { matcher, isNegated } = getAssertNodeInfo(node); + + if (!matcher) { + return false; + } + + return isNegated + ? ABSENCE_MATCHERS.includes(matcher) + : PRESENCE_MATCHERS.includes(matcher); + }, + + isAbsenceAssert(node) { + const { matcher, isNegated } = getAssertNodeInfo(node); + + if (!matcher) { + return false; + } + + return isNegated + ? PRESENCE_MATCHERS.includes(matcher) + : ABSENCE_MATCHERS.includes(matcher); + }, + /** * Determines whether a given MemberExpression node is a presence assert * From 691dc28c71997dab0e91c313414687238f2085e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 5 Nov 2020 11:32:14 +0100 Subject: [PATCH 2/4] refactor(prefer-presence-queries): use presence/absence helpers --- lib/detect-testing-library-utils.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index a6faee9d..f8a6f59f 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -145,6 +145,13 @@ export function detectTestingLibraryUtils< return this.isGetByQuery(node) || this.isQueryByQuery(node); }, + /** + * 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); @@ -157,6 +164,13 @@ export function detectTestingLibraryUtils< : 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); From 86e827cc573c60013c22fccb060856a12c906b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Mon, 9 Nov 2020 13:47:07 +0100 Subject: [PATCH 3/4] refactor: rename boolean detection helpers --- lib/detect-testing-library-utils.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index f8a6f59f..be760d35 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -44,8 +44,8 @@ export type DetectionHelpers = { getCustomModuleImportNode: () => ImportModuleNode | null; getTestingLibraryImportName: () => string | undefined; getCustomModuleImportName: () => string | undefined; - getIsTestingLibraryImported: () => boolean; - getIsValidFilename: () => boolean; + isTestingLibraryImported: () => boolean; + isValidFilename: () => boolean; isGetByQuery: (node: TSESTree.Identifier) => boolean; isQueryByQuery: (node: TSESTree.Identifier) => boolean; isSyncQuery: (node: TSESTree.Identifier) => boolean; @@ -107,7 +107,7 @@ export function detectTestingLibraryUtils< * then this method will return `true` ONLY IF a testing-library package * or custom module are imported. */ - getIsTestingLibraryImported() { + isTestingLibraryImported() { if (!customModule) { return true; } @@ -119,7 +119,7 @@ export function detectTestingLibraryUtils< * Determines whether filename is valid or not for current file * being analyzed based on "testing-library/filename-pattern" setting. */ - getIsValidFilename() { + isValidFilename() { const fileName = context.getFilename(); return !!fileName.match(filenamePattern); }, @@ -225,9 +225,7 @@ export function detectTestingLibraryUtils< * Determines if file inspected meets all conditions to be reported by rules or not. */ canReportErrors() { - return ( - helpers.getIsTestingLibraryImported() && helpers.getIsValidFilename() - ); + return helpers.isTestingLibraryImported() && helpers.isValidFilename(); }, /** * Gets a string and verifies if it was imported/required by our custom module node From b428cc15384a217808c9d6e6429730d55ce48519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Mon, 9 Nov 2020 17:13:25 +0100 Subject: [PATCH 4/4] refactor: create helpers as separated functions --- lib/detect-testing-library-utils.ts | 320 +++++++++++++--------------- 1 file changed, 150 insertions(+), 170 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index be760d35..6ca0f180 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -81,189 +81,169 @@ export function detectTestingLibraryUtils< DEFAULT_FILENAME_PATTERN; // Helpers for Testing Library detection. - const helpers: DetectionHelpers = { - getTestingLibraryImportNode() { - return importedTestingLibraryNode; - }, - getCustomModuleImportNode() { - return importedCustomModuleNode; - }, - getTestingLibraryImportName() { - return getImportModuleName(importedTestingLibraryNode); - }, - getCustomModuleImportName() { - return getImportModuleName(importedCustomModuleNode); - }, - /** - * 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 - * custom modules. - * - * However, there is a setting to customize the module where TL utils can - * be imported from: "testing-library/module". If this setting is enabled, - * then this method will return `true` ONLY IF a testing-library package - * or custom module are imported. - */ - isTestingLibraryImported() { - if (!customModule) { - return true; - } - - return !!importedTestingLibraryNode || !!importedCustomModuleNode; - }, - - /** - * Determines whether filename is valid or not for current file - * being analyzed based on "testing-library/filename-pattern" setting. - */ - isValidFilename() { - const fileName = context.getFilename(); - return !!fileName.match(filenamePattern); - }, - - /** - * Determines whether a given node is `getBy*` or `getAllBy*` query variant or not. - */ - isGetByQuery(node) { - return !!node.name.match(/^get(All)?By.+$/); - }, + const getTestingLibraryImportNode: DetectionHelpers['getTestingLibraryImportNode'] = () => { + return importedTestingLibraryNode; + }; - /** - * Determines whether a given node is `queryBy*` or `queryAllBy*` query variant or not. - */ - isQueryByQuery(node) { - return !!node.name.match(/^query(All)?By.+$/); - }, + const getCustomModuleImportNode: DetectionHelpers['getCustomModuleImportNode'] = () => { + return importedCustomModuleNode; + }; - /** - * Determines whether a given node is sync query or not. - */ - isSyncQuery(node) { - return this.isGetByQuery(node) || this.isQueryByQuery(node); - }, + const getTestingLibraryImportName: DetectionHelpers['getTestingLibraryImportName'] = () => { + return getImportModuleName(importedTestingLibraryNode); + }; - /** - * 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); + const getCustomModuleImportName: DetectionHelpers['getCustomModuleImportName'] = () => { + return getImportModuleName(importedCustomModuleNode); + }; + /** + * 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 + * custom modules. + * + * However, there is a setting to customize the module where TL utils can + * be imported from: "testing-library/module". If this setting is enabled, + * then this method will return `true` ONLY IF a testing-library package + * or custom module are imported. + */ + const isTestingLibraryImported: DetectionHelpers['isTestingLibraryImported'] = () => { + if (!customModule) { + return true; + } + + return !!importedTestingLibraryNode || !!importedCustomModuleNode; + }; - if (!matcher) { - return false; - } + /** + * Determines whether filename is valid or not for current file + * being analyzed based on "testing-library/filename-pattern" setting. + */ + const isValidFilename: DetectionHelpers['isValidFilename'] = () => { + const fileName = context.getFilename(); + return !!fileName.match(filenamePattern); + }; - return isNegated - ? ABSENCE_MATCHERS.includes(matcher) - : PRESENCE_MATCHERS.includes(matcher); - }, + /** + * Determines whether a given node is `getBy*` or `getAllBy*` query variant or not. + */ + const isGetByQuery: DetectionHelpers['isGetByQuery'] = (node) => { + return !!node.name.match(/^get(All)?By.+$/); + }; - /** - * 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); + /** + * Determines whether a given node is `queryBy*` or `queryAllBy*` query variant or not. + */ + const isQueryByQuery: DetectionHelpers['isQueryByQuery'] = (node) => { + return !!node.name.match(/^query(All)?By.+$/); + }; - if (!matcher) { - return false; - } + /** + * Determines whether a given node is sync query or not. + */ + const isSyncQuery: DetectionHelpers['isSyncQuery'] = (node) => { + return isGetByQuery(node) || isQueryByQuery(node); + }; - return isNegated - ? PRESENCE_MATCHERS.includes(matcher) - : ABSENCE_MATCHERS.includes(matcher); - }, + /** + * Determines whether a given MemberExpression node is a presence assert + * + * Presence asserts could have shape of: + * - expect(element).toBeInTheDocument() + * - expect(element).not.toBeNull() + */ + const isPresenceAssert: DetectionHelpers['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 a presence assert - * - * Presence asserts could have shape of: - * - expect(element).toBeInTheDocument() - * - expect(element).not.toBeNull() - */ - isPresenceAssert(node) { - const { matcher, isNegated } = getAssertNodeInfo(node); + /** + * Determines whether a given MemberExpression node is an absence assert + * + * Absence asserts could have shape of: + * - expect(element).toBeNull() + * - expect(element).not.toBeInTheDocument() + */ + const isAbsenceAssert: DetectionHelpers['isAbsenceAssert'] = (node) => { + const { matcher, isNegated } = getAssertNodeInfo(node); + + if (!matcher) { + return false; + } + + return isNegated + ? PRESENCE_MATCHERS.includes(matcher) + : ABSENCE_MATCHERS.includes(matcher); + }; - if (!matcher) { - return false; + /** + * Gets a string and verifies if it was imported/required by our custom module node + */ + const findImportedUtilSpecifier: DetectionHelpers['findImportedUtilSpecifier'] = ( + specifierName + ) => { + const node = getCustomModuleImportNode() ?? getTestingLibraryImportNode(); + if (!node) { + return null; + } + if (isImportDeclaration(node)) { + const namedExport = node.specifiers.find( + (n) => isImportSpecifier(n) && n.imported.name === specifierName + ); + // it is "import { foo [as alias] } from 'baz'"" + if (namedExport) { + return namedExport; } - - 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; + // it could be "import * as rtl from 'baz'" + return node.specifiers.find((n) => isImportNamespaceSpecifier(n)); + } else { + const requireNode = node.parent as TSESTree.VariableDeclarator; + if (ASTUtils.isIdentifier(requireNode.id)) { + // this is const rtl = require('foo') + return requireNode.id; } + // this should be const { something } = require('foo') + const destructuring = requireNode.id as TSESTree.ObjectPattern; + const property = destructuring.properties.find( + (n) => + isProperty(n) && + ASTUtils.isIdentifier(n.key) && + n.key.name === specifierName + ); + return (property as TSESTree.Property).key as TSESTree.Identifier; + } + }; - return isNegated - ? PRESENCE_MATCHERS.includes(matcher) - : ABSENCE_MATCHERS.includes(matcher); - }, + /** + * Determines if file inspected meets all conditions to be reported by rules or not. + */ + const canReportErrors: DetectionHelpers['canReportErrors'] = () => { + return isTestingLibraryImported() && isValidFilename(); + }; - /** - * Determines if file inspected meets all conditions to be reported by rules or not. - */ - canReportErrors() { - return helpers.isTestingLibraryImported() && helpers.isValidFilename(); - }, - /** - * Gets a string and verifies if it was imported/required by our custom module node - */ - findImportedUtilSpecifier(specifierName: string) { - const node = - helpers.getCustomModuleImportNode() ?? - helpers.getTestingLibraryImportNode(); - if (!node) { - return null; - } - if (isImportDeclaration(node)) { - const namedExport = node.specifiers.find( - (n) => isImportSpecifier(n) && n.imported.name === specifierName - ); - // it is "import { foo [as alias] } from 'baz'"" - if (namedExport) { - return namedExport; - } - // it could be "import * as rtl from 'baz'" - return node.specifiers.find((n) => isImportNamespaceSpecifier(n)); - } else { - const requireNode = node.parent as TSESTree.VariableDeclarator; - if (ASTUtils.isIdentifier(requireNode.id)) { - // this is const rtl = require('foo') - return requireNode.id; - } - // this should be const { something } = require('foo') - const destructuring = requireNode.id as TSESTree.ObjectPattern; - const property = destructuring.properties.find( - (n) => - isProperty(n) && - ASTUtils.isIdentifier(n.key) && - n.key.name === specifierName - ); - return (property as TSESTree.Property).key as TSESTree.Identifier; - } - }, + const helpers = { + getTestingLibraryImportNode, + getCustomModuleImportNode, + getTestingLibraryImportName, + getCustomModuleImportName, + isTestingLibraryImported, + isValidFilename, + isGetByQuery, + isQueryByQuery, + isSyncQuery, + isPresenceAssert, + isAbsenceAssert, + canReportErrors, + findImportedUtilSpecifier, }; // Instructions for Testing Library detection. @@ -344,7 +324,7 @@ export function detectTestingLibraryUtils< detectionInstructions[instruction](node); } - if (helpers.canReportErrors() && ruleInstructions[instruction]) { + if (canReportErrors() && ruleInstructions[instruction]) { return ruleInstructions[instruction](node); } };