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

feat: 🎸 new no-wait-for-snapshot rule #223

Merged
merged 1 commit into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
[![Tweet][tweet-badge]][tweet-url]

<!-- ALL-CONTRIBUTORS-BADGE:START - Do not remove or modify this section -->

[![All Contributors](https://img.shields.io/badge/all_contributors-31-orange.svg?style=flat-square)](#contributors-)

<!-- ALL-CONTRIBUTORS-BADGE:END -->

## Installation
Expand Down Expand Up @@ -143,6 +145,7 @@ To enable this configuration use the `extends` property in your
| [no-manual-cleanup](docs/rules/no-manual-cleanup.md) | Disallow the use of `cleanup` | | |
| [no-render-in-setup](docs/rules/no-render-in-setup.md) | Disallow the use of `render` in setup functions | | |
| [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | | |
| [no-wait-for-snapshot](docs/rules/no-wait-for-snapshot.md) | Ensures no snapshot is generated inside of a `waitFor` call | | |
| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | |
| [prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] |
| [prefer-presence-queries](docs/rules/prefer-presence-queries.md) | Enforce specific queries when checking element is present or not | | |
Expand Down Expand Up @@ -222,6 +225,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d

<!-- markdownlint-enable -->
<!-- prettier-ignore-end -->

<!-- ALL-CONTRIBUTORS-LIST:END -->

This project follows the [all-contributors](https://github.com/all-contributors/all-contributors) specification. Contributions of any kind welcome!
56 changes: 56 additions & 0 deletions docs/rules/no-wait-for-snapshot.test.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# Ensures no snapshot is generated inside of a `wait` call' (no-wait-for-snapshot)

Ensure that no calls to `toMatchSnapshot` or `toMatchInlineSnapshot` are made from within a `waitFor` method (or any of the other async utility methods).

## Rule Details

The `waitFor()` method runs in a timer loop. So it'll retry every n amount of time.
If a snapshot is generated inside the wait condition, jest will generate one snapshot per loop.

The problem then is the amount of loop ran until the condition is met will vary between different computers (or CI machines). This leads to tests that will regenerate a lot of snapshots until the condition is matched when devs run those tests locally updating the snapshots; e.g devs cannot run `jest -u` locally or it'll generate a lot of invalid snapshots who'll fail during CI.

Note that this lint rule prevents from generating a snapshot from within any of the [async utility methods](https://testing-library.com/docs/dom-testing-library/api-async).

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

```js
const foo = async () => {
// ...
await waitFor(() => expect(container).toMatchSnapshot());
// ...
};

const bar = async () => {
// ...
await waitFor(() => expect(container).toMatchInlineSnapshot());
// ...
};

const baz = async () => {
// ...
await wait(() => {
expect(container).toMatchSnapshot();
});
// ...
};
```

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

```js
const foo = () => {
// ...
expect(container).toMatchSnapshot();
// ...
};

const bar = () => {
// ...
expect(container).toMatchInlineSnapshot();
// ...
};
```

## Further Reading

- [Async Utilities](https://testing-library.com/docs/dom-testing-library/api-async)
2 changes: 2 additions & 0 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import noDomImport from './rules/no-dom-import';
import noManualCleanup from './rules/no-manual-cleanup';
import noRenderInSetup from './rules/no-render-in-setup';
import noWaitForEmptyCallback from './rules/no-wait-for-empty-callback';
import noWaitForSnapshot from './rules/no-wait-for-snapshot';
import preferExplicitAssert from './rules/prefer-explicit-assert';
import preferPresenceQueries from './rules/prefer-presence-queries';
import preferScreenQueries from './rules/prefer-screen-queries';
Expand All @@ -25,6 +26,7 @@ const rules = {
'no-manual-cleanup': noManualCleanup,
'no-render-in-setup': noRenderInSetup,
'no-wait-for-empty-callback': noWaitForEmptyCallback,
'no-wait-for-snapshot': noWaitForSnapshot,
'prefer-explicit-assert': preferExplicitAssert,
'prefer-find-by': preferFindBy,
'prefer-presence-queries': preferPresenceQueries,
Expand Down
2 changes: 2 additions & 0 deletions lib/node-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ export function findClosestCallExpressionNode(
return node;
}

if(!node.parent) return null
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've added this to avoid errors in case node.parent is null


return findClosestCallExpressionNode(node.parent);
}

Expand Down
132 changes: 132 additions & 0 deletions lib/rules/no-wait-for-snapshot.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { getDocsUrl, ASYNC_UTILS, LIBRARY_MODULES } from '../utils';
import {
findClosestCallExpressionNode,
isMemberExpression,
} from '../node-utils';

export const RULE_NAME = 'no-wait-for-snapshot';
export type MessageIds = 'noWaitForSnapshot';
type Options = [];

const ASYNC_UTILS_REGEXP = new RegExp(`^(${ASYNC_UTILS.join('|')})$`);
const SNAPSHOT_REGEXP = /^(toMatchSnapshot|toMatchInlineSnapshot)$/;

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
docs: {
description:
'Ensures no snapshot is generated inside of a `waitFor` call',
category: 'Best Practices',
recommended: 'warn',
},
messages: {
noWaitForSnapshot:
"A snapshot can't be generated inside of a `{{ name }}` call",
},
fixable: null,
schema: [],
},
defaultOptions: [],

create(context) {
const asyncUtilsUsage: Array<{
node: TSESTree.Identifier | TSESTree.MemberExpression;
name: string;
}> = [];
const importedAsyncUtils: string[] = [];
const snapshotUsage: TSESTree.Identifier[] = [];

return {
'ImportDeclaration > ImportSpecifier,ImportNamespaceSpecifier'(
node: TSESTree.Node
) {
const parent = node.parent as TSESTree.ImportDeclaration;

if (!LIBRARY_MODULES.includes(parent.source.value.toString())) return;

let name;
if (node.type === 'ImportSpecifier') {
name = node.imported.name;
}

if (node.type === 'ImportNamespaceSpecifier') {
name = node.local.name;
}

importedAsyncUtils.push(name);
},
[`CallExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`](
node: TSESTree.Identifier
) {
asyncUtilsUsage.push({ node, name: node.name });
},
[`CallExpression > MemberExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`](
node: TSESTree.Identifier
) {
const memberExpression = node.parent as TSESTree.MemberExpression;
const identifier = memberExpression.object as TSESTree.Identifier;
const memberExpressionName = identifier.name;

asyncUtilsUsage.push({
node: memberExpression,
name: memberExpressionName,
});
},
[`Identifier[name=${SNAPSHOT_REGEXP}]`](node: TSESTree.Identifier) {
snapshotUsage.push(node);
},
'Program:exit'() {
const testingLibraryUtilUsage = asyncUtilsUsage.filter(usage => {
if (isMemberExpression(usage.node)) {
const object = usage.node.object as TSESTree.Identifier;

return importedAsyncUtils.includes(object.name);
}

return importedAsyncUtils.includes(usage.name);
});

function getClosestAsyncUtil(
asyncUtilUsage: {
node: TSESTree.Identifier | TSESTree.MemberExpression;
name: string;
},
node: TSESTree.Node
) {
let callExpression = findClosestCallExpressionNode(node);
while (callExpression != null) {
Copy link
Collaborator

@gndelia gndelia Aug 27, 2020

Choose a reason for hiding this comment

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

I'd have hoped the linter configuration enforced !== over !=

I tend to avoid != completely but I'm not sure here 😅

Thoughts @Belco90 ?

edit: I know why the linter might have not complained: master does not run the linter in the PRs - that addition went to v4

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 think the linter allows for != because I'm checking against null. If you use != it's implicitly checking for undefined too

Copy link
Member

Choose a reason for hiding this comment

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

I think in this case is fine to compare against nil, and I understand that's the actual usage right?

if (callExpression.callee === asyncUtilUsage.node)
return asyncUtilUsage;
callExpression = findClosestCallExpressionNode(
callExpression.parent
);
}
return null;
}

snapshotUsage.forEach(node => {
testingLibraryUtilUsage.forEach(asyncUtilUsage => {
const closestAsyncUtil = getClosestAsyncUtil(asyncUtilUsage, node);
if (closestAsyncUtil != null) {
let name;
if (isMemberExpression(closestAsyncUtil.node)) {
name = (closestAsyncUtil.node.property as TSESTree.Identifier)
.name;
} else {
name = closestAsyncUtil.name;
}
context.report({
node,
messageId: 'noWaitForSnapshot',
data: { name },
});
}
});
});
},
};
},
});