adds no-all-mocks-methods rule
#204
Conversation
jaxee
force-pushed
the
fix-no-all-mocks-methods
branch
from
4896a8e
to
4a77dca
Compare
| The following patterns are not warnings: | ||
|
|
||
| ```js | ||
| jest.mock(); |
There was a problem hiding this comment.
Is there a better way to describe here the best practices?
There was a problem hiding this comment.
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.
| The following patterns are not warnings: | ||
|
|
||
| ```js | ||
| jest.mock(); |
There was a problem hiding this comment.
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.
| ```js | ||
| jest.mock(); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
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.
| context.report({ | ||
| node, | ||
| message: | ||
| 'Do not use {{method}} or related methods that are not explicit to your test. Instead, target individual mocks.', |
There was a problem hiding this comment.
I think you could be even more specific here. Suggestion:Do not use {{method}} or related methods that are not explicit to a single mock. Instead, clear, reset and restore mocks individually.
| code: `jest.mock()`, | ||
| }, | ||
| { | ||
| code: `jest.fn()`, |
There was a problem hiding this comment.
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.
cartogram
force-pushed
the
fix-no-all-mocks-methods
branch
from
4a77dca
to
4443169
Compare
cartogram
force-pushed
the
fix-no-all-mocks-methods
branch
from
c11eb34
to
00fc094
Compare
cartogram
force-pushed
the
fix-no-all-mocks-methods
branch
from
00fc094
to
e5bbddd
Compare
|
@jaxee Thanks for your help with this! 🙏 I am going to quickly wrap up the final things so that we can get this out of the PR list. 🙂 |
Closes part of https://github.com/Shopify/eslint-plugin-shopify/issues/181
For more information see: https://github.com/Shopify/web-foundation/blob/master/Best%20practices/Jest.md#best-practices
Do not use
jest.resetAllMocks(),jest.clearAllMocks(),jest.restoreAllMocks(), andjest.resetModules().First time adding a rule, feedback welcomed 🙏
cc @djirdehh