feat(rule): add new rule to validate jest.mock path existence
#1845
Conversation
Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
…ted LoCs Signed-off-by: hainenber <[email protected]>
…y.npmjs.org` URI instead Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
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?
Co-authored-by: Gareth Jones <[email protected]>
…`registry.npmjs.org` URI instead" This reverts commit c7d1901.
This reverts commit fee7e7a.
|
I think I'll go with |
…ons + faster return branches Signed-off-by: hainenber <[email protected]>
|
hi @G-Rath, I've addressed your review comments. Do lmk if there are further concerns :D |
src/rules/valid-mock-module-path.ts
Outdated
| if (!hasPossiblyModulePaths) { | ||
| throw { code: 'MODULE_NOT_FOUND' }; | ||
| } |
There was a problem hiding this comment.
this is a cute trick I'd not considered, but I'd like to get rid of the err: any and its best not to use errors for control flow - we should instead be able to move the context.report outside of the catch and use an early return to avoid triggering it
| ], | ||
| create( | ||
| context, | ||
| [{ moduleFileExtensions = ['.js', '.ts', '.tsx', '.jsx', '.json'] }], |
There was a problem hiding this comment.
can we add a few more tests for this option - we should have one for .json, and it would be good to have one that adds e.g. .css which'll serve to show both arbitrary extensions work and that it replaces rather than extends the default
(I know you've already got at least one test covering the latter, but its still nice to have another)
src/rules/valid-mock-module-path.ts
Outdated
| if (!hasPossiblyModulePaths) { | ||
| throw { code: 'MODULE_NOT_FOUND' }; | ||
| } | ||
| } catch (err: any) { |
There was a problem hiding this comment.
I'd prefer we stick with unknown rather than any, and use a cast to access code.
That's slightly safer because we can't mistakenly use err elsewhere without more casting
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
|
hi @G-Rath, I've refactored per the 2nd round of review, including these items:
|
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.