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.

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

Move rules settings to ESLint shared config: refactor await-fire-event #265

Merged
merged 17 commits into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ To enable this configuration use the `extends` property in your
| -------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | ------------------ |
| [await-async-query](docs/rules/await-async-query.md) | Enforce promises from async queries to be handled | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | |
| [await-async-utils](docs/rules/await-async-utils.md) | Enforce async utils to be awaited properly | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | |
| [await-fire-event](docs/rules/await-fire-event.md) | Enforce async fire event methods to be awaited | ![vue-badge][] | |
| [await-fire-event](docs/rules/await-fire-event.md) | Enforce promises from fire event methods to be handled | ![vue-badge][] | |
| [consistent-data-testid](docs/rules/consistent-data-testid.md) | Ensure `data-testid` values match a provided regex. | | |
| [no-await-sync-events](docs/rules/no-await-sync-events.md) | Disallow unnecessary `await` for sync events | | |
| [no-await-sync-query](docs/rules/no-await-sync-query.md) | Disallow unnecessary `await` for sync queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | |
Expand Down
35 changes: 27 additions & 8 deletions docs/rules/await-fire-event.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
# Enforce async fire event methods to be awaited (await-fire-event)
# Enforce promises from fire event methods to be handled (await-fire-event)

Ensure that promises returned by `fireEvent` methods are awaited
Ensure that promises returned by `fireEvent` methods are handled
properly.

## Rule Details

This rule aims to prevent users from forgetting to await `fireEvent`
methods when they are async.
This rule aims to prevent users from forgetting to handle promise returned from `fireEvent`
methods.

> ⚠️ `fireEvent` methods are async only on following Testing Library packages:
> - `@testing-library/vue` (supported by this plugin)
> - `@testing-library/svelte` (not supported yet by this plugin)

Examples of **incorrect** code for this rule:

Expand All @@ -15,6 +19,12 @@ fireEvent.click(getByText('Click me'));

fireEvent.focus(getByLabelText('username'));
fireEvent.blur(getByLabelText('username'));

// wrap a fireEvent method within a function...
function triggerEvent() {
return fireEvent.click(button);
}
triggerEvent(); // ...but not handling promise from it is incorrect too
```

Examples of **correct** code for this rule:
Expand All @@ -30,15 +40,24 @@ fireEvent.click(getByText('Click me')).then(() => {
});

// return the promise within a function is correct too!
function clickMeRegularFn() {
return fireEvent.click(getByText('Click me'));
}
const clickMeArrowFn = () => fireEvent.click(getByText('Click me'));

// wrap a fireEvent method within a function...
function triggerEvent() {
return fireEvent.click(button);
}
await triggerEvent(); // ...and handling promise from it is correct also

// using `Promise.all` or `Promise.allSettled` with an array of promises is valid
await Promise.all([
fireEvent.focus(getByLabelText('username')),
fireEvent.blur(getByLabelText('username')),
]);
```

## When Not To Use It

`fireEvent` methods are only async in Vue Testing Library so if you are using another Testing Library module, you shouldn't use this rule.
`fireEvent` methods are not async on all Testing Library packages. If you are not using Testing Library package with async fire event, you shouldn't use this rule.

## Further Reading

Expand Down
129 changes: 83 additions & 46 deletions lib/detect-testing-library-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,15 @@ import {
TSESTree,
} from '@typescript-eslint/experimental-utils';
import {
getImportModuleName,
getAssertNodeInfo,
isLiteral,
getImportModuleName,
ImportModuleNode,
isImportDeclaration,
isImportNamespaceSpecifier,
isImportSpecifier,
isLiteral,
isMemberExpression,
isProperty,
isCallExpression,
isObjectPattern,
} from './node-utils';
import { ABSENCE_MATCHERS, ASYNC_UTILS, PRESENCE_MATCHERS } from './utils';

Expand Down Expand Up @@ -54,6 +53,7 @@ export type DetectionHelpers = {
isSyncQuery: (node: TSESTree.Identifier) => boolean;
isAsyncQuery: (node: TSESTree.Identifier) => boolean;
isAsyncUtil: (node: TSESTree.Identifier) => boolean;
isFireEventMethod: (node: TSESTree.Identifier) => boolean;
isPresenceAssert: (node: TSESTree.MemberExpression) => boolean;
isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean;
canReportErrors: () => boolean;
Expand All @@ -67,6 +67,8 @@ export type DetectionHelpers = {

const DEFAULT_FILENAME_PATTERN = '^.*\\.(test|spec)\\.[jt]sx?$';

const FIRE_EVENT_NAME = 'fireEvent';

/**
* Enhances a given rule `create` with helpers to detect Testing Library utils.
*/
Expand All @@ -88,6 +90,15 @@ export function detectTestingLibraryUtils<
context.settings['testing-library/filename-pattern'] ??
DEFAULT_FILENAME_PATTERN;

/**
* Determines whether aggressive 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)
*/
const isAggressiveReportingEnabled = () => !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.

I extracted this into this simple function for adding some syntactic sugar around "aggressive reporting" concept. I'll create a separate PR soon where I'll use this in isNodeComingFromTestingLibrary.


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

Expand Down Expand Up @@ -176,6 +187,58 @@ export function detectTestingLibraryUtils<
return ASYNC_UTILS.includes(node.name);
};

/**
* Determines whether a given node is fireEvent method or not
*/
const isFireEventMethod: DetectionHelpers['isFireEventMethod'] = (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.

Extracted from prefer-user-event! I tweaked it a little bit to improve some edge cases.

const fireEventUtil = findImportedUtilSpecifier(FIRE_EVENT_NAME);
let fireEventUtilName: string | undefined;

if (fireEventUtil) {
fireEventUtilName = ASTUtils.isIdentifier(fireEventUtil)
? fireEventUtil.name
: fireEventUtil.local.name;
} else if (isAggressiveReportingEnabled()) {
fireEventUtilName = FIRE_EVENT_NAME;
}

if (!fireEventUtilName) {
return false;
}

const parentMemberExpression:
| TSESTree.MemberExpression
| undefined = isMemberExpression(node.parent) ? node.parent : undefined;

if (!parentMemberExpression) {
return false;
}

// make sure that given node it's not fireEvent object itself
if (
[fireEventUtilName, FIRE_EVENT_NAME].includes(node.name) ||
(ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === node.name)
) {
return false;
}

// check fireEvent.click() usage
const regularCall =
ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === fireEventUtilName;

// check testingLibraryUtils.fireEvent.click() usage
const wildcardCall =
isMemberExpression(parentMemberExpression.object) &&
ASTUtils.isIdentifier(parentMemberExpression.object.object) &&
parentMemberExpression.object.object.name === fireEventUtilName &&
ASTUtils.isIdentifier(parentMemberExpression.object.property) &&
parentMemberExpression.object.property.name === FIRE_EVENT_NAME;

return regularCall || wildcardCall;
};

/**
* Determines whether a given MemberExpression node is a presence assert
*
Expand Down Expand Up @@ -215,7 +278,8 @@ export function detectTestingLibraryUtils<
};

/**
* Gets a string and verifies if it was imported/required by our custom module node
* Gets a string and verifies if it was imported/required by Testing Library
* related module.
*/
const findImportedUtilSpecifier: DetectionHelpers['findImportedUtilSpecifier'] = (
specifierName
Expand Down Expand Up @@ -260,52 +324,24 @@ export function detectTestingLibraryUtils<
};
/**
* Takes a MemberExpression or an Identifier and verifies if its name comes from the import in TL
* @param node a MemberExpression (in "foo.property" it would be property) or an Identifier (it should be provided from a CallExpression, for example "foo()")
* @param node a MemberExpression (in "foo.property" it would be property) or an Identifier
*/
const isNodeComingFromTestingLibrary: DetectionHelpers['isNodeComingFromTestingLibrary'] = (
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 realized this one was doing basically the same as findImportedUtilSpecifier so I refactored it.

node: TSESTree.MemberExpression | TSESTree.Identifier
) => {
const importOrRequire =
getCustomModuleImportNode() ?? getTestingLibraryImportNode();
if (!importOrRequire) {
return false;
}
let identifierName: string | undefined;

if (ASTUtils.isIdentifier(node)) {
if (isImportDeclaration(importOrRequire)) {
return importOrRequire.specifiers.some(
(s) => isImportSpecifier(s) && s.local.name === node.name
);
} else {
return (
ASTUtils.isVariableDeclarator(importOrRequire.parent) &&
isObjectPattern(importOrRequire.parent.id) &&
importOrRequire.parent.id.properties.some(
(p) =>
isProperty(p) &&
ASTUtils.isIdentifier(p.key) &&
ASTUtils.isIdentifier(p.value) &&
p.value.name === node.name
)
);
}
} else {
if (!ASTUtils.isIdentifier(node.object)) {
return false;
}
if (isImportDeclaration(importOrRequire)) {
return (
isImportDeclaration(importOrRequire) &&
isImportNamespaceSpecifier(importOrRequire.specifiers[0]) &&
node.object.name === importOrRequire.specifiers[0].local.name
);
}
return (
isCallExpression(importOrRequire) &&
ASTUtils.isVariableDeclarator(importOrRequire.parent) &&
ASTUtils.isIdentifier(importOrRequire.parent.id) &&
node.object.name === importOrRequire.parent.id.name
);
identifierName = node.name;
} else if (ASTUtils.isIdentifier(node.object)) {
identifierName = node.object.name;
}

if (!identifierName) {
return;
}

return !!findImportedUtilSpecifier(identifierName);
};

const helpers = {
Expand All @@ -321,6 +357,7 @@ export function detectTestingLibraryUtils<
isSyncQuery,
isAsyncQuery,
isAsyncUtil,
isFireEventMethod,
isPresenceAssert,
isAbsenceAssert,
canReportErrors,
Expand Down
24 changes: 15 additions & 9 deletions lib/node-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ const ValidLeftHandSideExpressions = [
AST_NODE_TYPES.ObjectExpression,
AST_NODE_TYPES.ObjectPattern,
AST_NODE_TYPES.Super,
AST_NODE_TYPES.TemplateLiteral,
Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicated in the list

AST_NODE_TYPES.ThisExpression,
AST_NODE_TYPES.TSNullKeyword,
AST_NODE_TYPES.TaggedTemplateExpression,
Expand Down Expand Up @@ -132,7 +131,7 @@ export function findClosestCallExpressionNode(
return null;
}

return findClosestCallExpressionNode(node.parent);
return findClosestCallExpressionNode(node.parent, shouldRestrictInnerScope);
Copy link
Member Author

Choose a reason for hiding this comment

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

Bug difficult to spot here! Took me a while to find it.

}

export function findClosestCallNode(
Expand Down Expand Up @@ -232,15 +231,22 @@ export function isPromiseAllSettled(node: TSESTree.CallExpression): boolean {
);
}

/**
* Determines whether a given node belongs to handled Promise.all or Promise.allSettled
* array expression.
*/
export function isPromisesArrayResolved(node: TSESTree.Node): boolean {
const parent = node.parent;
const closestCallExpression = findClosestCallExpressionNode(node, true);

if (!closestCallExpression) {
return false;
}

return (
isCallExpression(parent) &&
isArrayExpression(parent.parent) &&
isCallExpression(parent.parent.parent) &&
(isPromiseAll(parent.parent.parent) ||
isPromiseAllSettled(parent.parent.parent))
isArrayExpression(closestCallExpression.parent) &&
isCallExpression(closestCallExpression.parent.parent) &&
(isPromiseAll(closestCallExpression.parent.parent) ||
isPromiseAllSettled(closestCallExpression.parent.parent))
);
}

Expand All @@ -253,7 +259,7 @@ export function isPromisesArrayResolved(node: TSESTree.Node): boolean {
* - it belongs to the `Promise.allSettled` method
* - it's chained with the `then` method
* - it's returned from a function
* - has `resolves` or `rejects`
* - has `resolves` or `rejects` jest methods
*/
export function isPromiseHandled(nodeIdentifier: TSESTree.Identifier): boolean {
const closestCallExpressionNode = findClosestCallExpressionNode(
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/await-async-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export default createTestingLibraryRule<Options, MessageIds>({
);

if (references && references.length === 0) {
if (!isPromiseHandled(node as TSESTree.Identifier)) {
if (!isPromiseHandled(node)) {
return context.report({
node,
messageId: 'awaitAsyncUtil',
Expand Down
Loading