Skip to content

Move rules settings to ESLint shared config: Aggressive Render Reporting + refactor render-result-naming-convention #280

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

Merged
merged 7 commits into from
Mar 3, 2021
Merged

Move rules settings to ESLint shared config: Aggressive Render Reporting + refactor render-result-naming-convention #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8684ab8
docs: add comments to main parts to be modified
Belco90 Jan 23, 2021
5a2d128
refactor(render-result-naming-convention): first approach for new helper
Belco90 Feb 28, 2021
bc8de3d
refactor(aggressive-render): update criteria to consider valid renders
Belco90 Mar 1, 2021
7c058ea
feat(aggressive-render): keep aggressive module reporting in mind
Belco90 Mar 1, 2021
4b5ab36
test(render-result-naming-convention): move valid to invalid tests
Belco90 Mar 1, 2021
845ce69
docs(aggressive-reporting): improve jsdocs
Belco90 Mar 2, 2021
9a05e35
test(create-testing-library-rule): cases for render
Belco90 Mar 2, 2021
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
77 changes: 70 additions & 7 deletions lib/detect-testing-library-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
} from '@typescript-eslint/experimental-utils';
import {
getAssertNodeInfo,
getIdentifierNode,
getImportModuleName,
ImportModuleNode,
isImportDeclaration,
Expand All @@ -24,6 +25,7 @@ import {
export type TestingLibrarySettings = {
'testing-library/module'?: string;
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 want to rename this one to testing-library/utils-module. I'll do this in the next PR, which will be just to apply some tweaks.

'testing-library/filename-pattern'?: string;
'testing-library/custom-renders'?: string[];
};

export type TestingLibraryContext<
Expand Down Expand Up @@ -60,6 +62,7 @@ export type DetectionHelpers = {
isCustomQuery: (node: TSESTree.Identifier) => boolean;
isAsyncUtil: (node: TSESTree.Identifier) => boolean;
isFireEventMethod: (node: TSESTree.Identifier) => boolean;
isRenderUtil: (node: TSESTree.Node) => boolean;
isPresenceAssert: (node: TSESTree.MemberExpression) => boolean;
isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean;
canReportErrors: () => boolean;
Expand All @@ -74,6 +77,7 @@ export type DetectionHelpers = {
const DEFAULT_FILENAME_PATTERN = '^.*\\.(test|spec)\\.[jt]sx?$';

const FIRE_EVENT_NAME = 'fireEvent';
const RENDER_NAME = 'render';

/**
* Enhances a given rule `create` with helpers to detect Testing Library utils.
Expand All @@ -95,15 +99,31 @@ export function detectTestingLibraryUtils<
const filenamePattern =
context.settings['testing-library/filename-pattern'] ??
DEFAULT_FILENAME_PATTERN;
const customRenders = context.settings['testing-library/custom-renders'];

/**
* Determines whether aggressive reporting is enabled or not.
* Determines whether aggressive module reporting is enabled or not.
*
* Aggressive reporting is considered as enabled when:
* - custom module is not set (so we need to assume everything
* matching TL utils is related to TL no matter where it was imported from)
* This aggressive reporting mechanism is considered as enabled when custom
* module is not set, so we need to assume everything matching Testing
* Library utils is related to Testing Library no matter from where module
* they are coming from. Otherwise, this aggressive reporting mechanism is
* opted-out in favour to report only those utils coming from Testing
* Library package or custom module set up on settings.
*/
const isAggressiveReportingEnabled = () => !customModule;
const isAggressiveModuleReportingEnabled = () => !customModule;
Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming this method to clarify that it's just checking the Aggressive Reporting mechanism related to module from where Testing Library utils can be imported.


/**
* Determines whether aggressive render reporting is enabled or not.
*
* This aggressive reporting mechanism is considered as enabled when custom
* renders are not set, so we need to assume every method containing
* "render" is a valid Testing Library `render`. Otherwise, this aggressive
* reporting mechanism is opted-out in favour to report only `render` or
* names set up on custom renders setting.
*/
const isAggressiveRenderReportingEnabled = () =>
!Array.isArray(customRenders) || customRenders.length === 0;

// Helpers for Testing Library detection.
const getTestingLibraryImportNode: DetectionHelpers['getTestingLibraryImportNode'] = () => {
Expand Down Expand Up @@ -135,7 +155,7 @@ export function detectTestingLibraryUtils<
* or custom module are imported.
*/
const isTestingLibraryImported: DetectionHelpers['isTestingLibraryImported'] = () => {
if (isAggressiveReportingEnabled()) {
if (isAggressiveModuleReportingEnabled()) {
return true;
}

Expand Down Expand Up @@ -215,7 +235,7 @@ export function detectTestingLibraryUtils<
fireEventUtilName = ASTUtils.isIdentifier(fireEventUtil)
? fireEventUtil.name
: fireEventUtil.local.name;
} else if (isAggressiveReportingEnabled()) {
} else if (isAggressiveModuleReportingEnabled()) {
fireEventUtilName = FIRE_EVENT_NAME;
}

Expand Down Expand Up @@ -256,6 +276,48 @@ export function detectTestingLibraryUtils<
return regularCall || wildcardCall;
};

/**
* Determines whether a given node is a valid render util or not.
*
* A node will be interpreted as a valid render based on two conditions:
* the name matches with a valid "render" option, and the node is coming
* from Testing Library module. This depends on:
*
* - Aggressive render reporting: if enabled, then every node name
* containing "render" will be assumed as Testing Library render util.
* Otherwise, it means `custom-modules` has been set up, so only those nodes
* named as "render" or some of the `custom-modules` options will be
* considered as Testing Library render util.
* - Aggressive module reporting: if enabled, then it doesn't matter from
* where the given node was imported from as it will be considered part of
* Testing Library. Otherwise, it means `custom-module` has been set up, so
* only those nodes coming from Testing Library will be considered as valid.
*/
const isRenderUtil: DetectionHelpers['isRenderUtil'] = (node) => {
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 is the new helper to now if some node is a valid render method. It wasn't that complex now that we had all the other helpers! This is enough for now, but maybe some isComingFromRender could be useful in te future.

const identifier = getIdentifierNode(node);

if (!identifier) {
return false;
}

const isNameMatching = (function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why making it an IIFE and not a regular function? 🙂

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 was just a fancy way of assigning isNameMatching value to a const rather than a let 😅

if (isAggressiveRenderReportingEnabled()) {
return identifier.name.toLowerCase().includes(RENDER_NAME);
}

return [RENDER_NAME, ...customRenders].includes(identifier.name);
})();

if (!isNameMatching) {
return false;
}

return (
isAggressiveModuleReportingEnabled() ||
isNodeComingFromTestingLibrary(identifier)
);
};

/**
* Determines whether a given MemberExpression node is a presence assert
*
Expand Down Expand Up @@ -376,6 +438,7 @@ export function detectTestingLibraryUtils<
isCustomQuery,
isAsyncUtil,
isFireEventMethod,
isRenderUtil,
isPresenceAssert,
isAbsenceAssert,
canReportErrors,
Expand Down
113 changes: 14 additions & 99 deletions lib/rules/render-result-naming-convention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,18 @@
import {
ESLintUtils,
TSESTree,
ASTUtils,
} from '@typescript-eslint/experimental-utils';
import { getDocsUrl, hasTestingLibraryImportModule } from '../utils';
import {
isCallExpression,
isImportSpecifier,
isMemberExpression,
isObjectPattern,
isRenderVariableDeclarator,
} from '../node-utils';
import { createTestingLibraryRule } from '../create-testing-library-rule';
import { isObjectPattern } from '../node-utils';
import { ASTUtils } from '@typescript-eslint/experimental-utils';

export const RULE_NAME = 'render-result-naming-convention';
export type MessageIds = 'renderResultNamingConvention';
type Options = [{ renderFunctions?: string[] }];

type Options = [];

const ALLOWED_VAR_NAMES = ['view', 'utils'];
const ALLOWED_VAR_NAMES_TEXT = ALLOWED_VAR_NAMES.map(
(name) => `\`${name}\``
).join(', ');

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
export default createTestingLibraryRule<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'suggestion',
Expand All @@ -31,103 +22,27 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
recommended: false,
},
messages: {
renderResultNamingConvention: `\`{{ varName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or call it using one of the valid choices: ${ALLOWED_VAR_NAMES_TEXT}`,
renderResultNamingConvention: `\`{{ renderResultName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or name it using one of: ${ALLOWED_VAR_NAMES_TEXT}`,
},
fixable: null,
schema: [
{
type: 'object',
properties: {
renderFunctions: {
type: 'array',
},
},
},
],
schema: [],
},
defaultOptions: [
{
renderFunctions: [],
},
],

create(context, [options]) {
const { renderFunctions } = options;
let renderAlias: string | undefined;
let wildcardImportName: string | undefined;
defaultOptions: [],

create(context, _, helpers) {
return {
// check named imports
ImportDeclaration(node: TSESTree.ImportDeclaration) {
if (!hasTestingLibraryImportModule(node)) {
return;
}
const renderImport = node.specifiers.find(
(node) => isImportSpecifier(node) && node.imported.name === 'render'
);

if (!renderImport) {
return;
}

renderAlias = renderImport.local.name;
},
// check wildcard imports
'ImportDeclaration ImportNamespaceSpecifier'(
node: TSESTree.ImportNamespaceSpecifier
) {
if (
!hasTestingLibraryImportModule(
node.parent as TSESTree.ImportDeclaration
)
) {
VariableDeclarator(node) {
if (!helpers.isRenderUtil(node.init)) {
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 can't believe how simple this rule is now 🤯

Anyway, the rule is not checking nested/wrapper render calls like:

const setup = () => render(<Foo />)

// ...

const wrapper = setup()

I'll include this in a separate PR so I avoid adding to many things to this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, the developer experience is way nicer!

return;
}

wildcardImportName = node.local.name;
},
VariableDeclarator(node: TSESTree.VariableDeclarator) {
// check if destructuring return value from render
if (isObjectPattern(node.id)) {
return;
}

const isValidRenderDeclarator = isRenderVariableDeclarator(node, [
...renderFunctions,
renderAlias,
]);
const isValidWildcardImport = !!wildcardImportName;

// check if is a Testing Library related import
if (!isValidRenderDeclarator && !isValidWildcardImport) {
return;
}

const renderFunctionName =
isCallExpression(node.init) &&
ASTUtils.isIdentifier(node.init.callee) &&
node.init.callee.name;

const renderFunctionObjectName =
isCallExpression(node.init) &&
isMemberExpression(node.init.callee) &&
ASTUtils.isIdentifier(node.init.callee.property) &&
ASTUtils.isIdentifier(node.init.callee.object) &&
node.init.callee.property.name === 'render' &&
node.init.callee.object.name;

const isRenderAlias = !!renderAlias;
const isCustomRender = renderFunctions.includes(renderFunctionName);
const isWildCardRender =
renderFunctionObjectName &&
renderFunctionObjectName === wildcardImportName;

// check if is a qualified render function
if (!isRenderAlias && !isCustomRender && !isWildCardRender) {
return;
}

const renderResultName = ASTUtils.isIdentifier(node.id) && node.id.name;

const isAllowedRenderResultName = ALLOWED_VAR_NAMES.includes(
renderResultName
);
Expand All @@ -141,7 +56,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
node,
messageId: 'renderResultNamingConvention',
data: {
varName: renderResultName,
renderResultName,
},
});
},
Expand Down
Loading