Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

adds no-all-mocks-methods rule #204

Merged
merged 3 commits into from
Nov 14, 2019
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@

## Unreleased

- `shopify/no-all-mocks-methods` ([#204](https://github.com/Shopify/eslint-plugin-shopify/pull/204))
- `jest/valid-title`
- `jest/prefer-hooks-on-top`
- `jest/require-top-level-describe`

## [32.0.0] - 2019-11-05

- Enforce new-lines between groups import groups ([#409](https://github.com/Shopify/eslint-plugin-shopify/pull/409))

## [31.0.0] - 2019-10-23

### Using `@typescript-eslint/parser` and `@typescript-eslint/eslint-plugin`
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ This plugin provides the following custom rules, which are included as appropria
- [prefer-class-properties](docs/rules/prefer-class-properties.md): Prefer class properties to assignment of literals in constructors.
- [prefer-early-return](docs/rules/prefer-early-return.md): Prefer early returns over full-body conditional wrapping in function declarations.
- [no-ancestor-directory-import](docs/rules/no-ancestor-directory-import.md): Prefer imports from within a directory extend to the file from where they are importing without relying on an index file.
- [no-all-mocks-methods](docs/rules/no-all-mocks-methods.md): Disallows jest allMocks methods.
- [prefer-module-scope-constants](docs/rules/prefer-module-scope-constants.md): Prefer that screaming snake case variables always be defined using `const`, and always appear at module scope.
- [prefer-twine](docs/rules/prefer-twine.md): Prefer Twine over Bindings as the name for twine imports.
- [react-hooks-strict-return](docs/rules/react-hooks-strict-return.md): Restrict the number of returned items from React hooks.
Expand Down
35 changes: 35 additions & 0 deletions docs/rules/jest/no-all-mocks-methods.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Disallows jest allMocks methods.

This rule discourages the use of overly broad Jest methods such as `resetAllMocks`, `clearAllMocks`, `restoreAllMocks` and `resetModules`.

## Rule Details

These methods are discouraged because there should be explicit connections to the contents of your test file. Mocks should be reset/cleared/restored individually based on the purpose of your test suite.

The following patterns are considered warnings:

```js
jest.resetAllMocks();
```

```js
jest.clearAllMocks();
```

```js
jest.restoreAllMocks();
```

```js
jest.resetModules();
```

The following patterns are not warnings:

```js
jest.mock();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to describe here the best practices?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, you could go one step further and fill in the alternative for each method you are restricting here. For example, showing a full example using mockReset(); as the alternative to resetAllMocks.

```

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, there is a note here about when to its safe to disable this rule. Its pretty template-y but I would add it for consistency.

## Further Reading

- [Shopify Jest Best Practices](https://github.com/Shopify/web-foundation/blob/master/Best%20practices/Jest.md#best-practices)
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module.exports = {
'binary-assignment-parens': require('./lib/rules/binary-assignment-parens'),
'class-property-semi': require('./lib/rules/class-property-semi'),
'images-no-direct-imports': require('./lib/rules/images-no-direct-imports'),
'jest/no-all-mocks-methods': require('./lib/rules/jest/no-all-mocks-methods'),
'jest/no-snapshots': require('./lib/rules/jest/no-snapshots'),
'jest/no-vague-titles': require('./lib/rules/jest/no-vague-titles'),
'jsx-no-complex-expressions': require('./lib/rules/jsx-no-complex-expressions'),
Expand Down
2 changes: 2 additions & 0 deletions lib/config/rules/shopify.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module.exports = {
'shopify/class-property-semi': 'error',
// Prevent images from being directly imported
'shopify/images-no-direct-imports': 'error',
// Disallow jest allMocks methods.
'shopify/jest/no-all-mocks-methods': 'off',
// Disallow jest snapshots.
'shopify/jest/no-snapshots': 'off',
// Disallow vague words in test statements.
Expand Down
38 changes: 38 additions & 0 deletions lib/rules/jest/no-all-mocks-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
module.exports = {
meta: {
docs: {
description: 'Disallows jest allMocks methods.',
category: 'Best Practices',
recommended: false,
uri:
'https://github.com/Shopify/eslint-plugin-shopify/blob/master/docs/rules/jest/no-all-mocks-methods.md',
},
},
messages: {
allMocksMethod:
'Do not use {{method}} or related methods that are not explicit to a single mock. Instead, clear, reset and restore mocks individually.',
},

create(context) {
return {
Identifier(node) {
if (isInvalidMocks(node.name)) {
context.report({
node,
messageId: 'allMocksMethod',
data: {method: node.name},
});
}
},
};
},
};

function isInvalidMocks(name) {
return [
'resetAllMocks',
'clearAllMocks',
'restoreAllMocks',
'resetModules',
].some((method) => method === name);
}
50 changes: 50 additions & 0 deletions tests/lib/rules/jest/no-all-mocks-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
const {RuleTester} = require('eslint');

const rule = require('../../../../lib/rules/jest/no-all-mocks-methods');

const ruleTester = new RuleTester();

ruleTester.run('no-all-mocks-methods', rule, {
valid: [
{
code: `jest.mock()`,
},
{
code: `jest.fn()`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule is fine as is, but you could check that the method is chained off the jest object. Right now, if there were a method in your application called clearAllMocks (completely unrelated to jest) that would be considered invalid. This is highly unlikely, but you could alter this rule to prevent against it.

At the same time, if you solve that problem, a second problem could occur if the methods were destructured off the jest object (const {clearAllMocks} = jest; or const clearAllMocks = jest.clearAllMocks or even somethingTotallyDifferent = jest.clearAllMocks). I think your rule will catch this right now.

You could protect against all of the above at the same time, but it is a lot more complex and I don't anticipate too many problems with the rule as is. However, it would be a fun challenge. I am also more than happy to pair again on it.

},
],
invalid: [
{
code: 'jest.resetAllMocks()',
errors: [
{
messageId: 'allMocksMethod',
},
],
},
{
code: 'jest.clearAllMocks()',
errors: [
{
messageId: 'allMocksMethod',
},
],
},
{
code: 'jest.restoreAllMocks()',
errors: [
{
messageId: 'allMocksMethod',
},
],
},
{
code: 'jest.resetModules()',
errors: [
{
messageId: 'allMocksMethod',
},
],
},
],
});