-
Notifications
You must be signed in to change notification settings - Fork 147
feat: 🎸 new no-wait-for-snapshot rule #223
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd have hoped the linter configuration enforced I tend to avoid 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the linter allows for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }, | ||
}); | ||
} | ||
}); | ||
}); | ||
}, | ||
}; | ||
}, | ||
}); |
There was a problem hiding this comment.
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
isnull