feat(rule): add new rule to validate jest.mock path existence
#1845
Conversation
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
…ted LoCs Signed-off-by: hainenber <dotronghai96@gmail.com>
…y.npmjs.org` URI instead Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
G-Rath
left a comment
G-Rath
left a comment
There was a problem hiding this comment.
looks good, just a few bits to cleanup - I'm also on the fence about the name: I think "mock" might be slightly better than "mocked" because we're dealing about something that is going to be mocked, rather something that has already been mocked, but then I'm not sure if it should be valid-mock-module-path or valid-module-mock-path 😅
also please revert the changes re link checking - I've landed #1846 which addresses the issue, and we don't need to update the package
| create(context) { | ||
| return { | ||
| CallExpression(node: TSESTree.CallExpression): void { | ||
| const { callee } = node; |
There was a problem hiding this comment.
nit: I don't think it's worth destructing this or property into variables given they're only used a couple of times
| if (!moduleName) { | ||
| throw new Error( | ||
| 'Cannot parse mocked module name from `jest.mock` - - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`', | ||
| ); | ||
| } |
There was a problem hiding this comment.
this is a very possible path as the requirement is "something other than a string is passed as the first argument", which includes variables but also silly stuff like an inline function (i.e. jest.mock(() => {}))
| throw { code: 'MODULE_NOT_FOUND' }; | ||
| } | ||
| } else { | ||
| require.resolve(moduleName.value); |
There was a problem hiding this comment.
we can do an early return to avoid this else, and if you invert the condition we can deindent the other (larger) code path
| // Skip over any unexpected issues when attempt to verify mocked module path. | ||
| // The list of possible errors is non-exhaustive. | ||
| /* istanbul ignore if */ | ||
| if (!['MODULE_NOT_FOUND', 'ENOENT'].includes(err.code)) { |
There was a problem hiding this comment.
I don't think we should be swallowing all other errors like this - we should either report or rethrow.
The two situations I can think of where we'd have an error is a bug in our code, or an issue with accessing the path.
Ideally for the former we'd want to rethrow, but the latter we probably want to report since its reasonable to expect Jest itself would have the same error - unfortunately I don't think we can reliably distinguish those situations so we probably just need to pick one action and change it later if it turns out to be noisy
There was a problem hiding this comment.
Okay so I think I'll change the code to rethrow unexpected errors since there are only fs and path-related errors, end user can resolve (no pun intended) by themselves.
| moduleName.value, | ||
| ); | ||
|
|
||
| const hasPossiblyModulePaths = ['', '.js', '.ts'] |
There was a problem hiding this comment.
This should probably be configurable, and I'm 98% sure this is related to moduleFileExtensions, so it might be best to have Jest v30s default as our default
(technically moduleNameMapper matters too, but that's complex enough that I think we can save that option for another time)
@SimenB can you confirm if I'm right?
|
I think I'll go with |
2 participants
Close #1584
Add new rule to verify whether
jest.mockin 1st argument, intended for a module and/or a local file, to exists or not.For local files, I've limited file checks for
.ts.and.jsfiles only as I don't want to increase the complexity further with combination of.cjs,.mtsand alike.New unit tests added.