-
Notifications
You must be signed in to change notification settings - Fork 234
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
feat(prefer-jest-mocked): add new rule #1599
Conversation
It looks like CI is broken. |
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.
thanks, looks like a good start
in addition to my specific comments:
- some of your tests have whitespace on their "empty" lines; would be great if you could remove that
- we should have a couple of cases that cover chained assertions and with
jest
in the middle (e.g.as unknown as string as unknown as jest.MockedFunction
andas unknown as jest.MockedFunction as string as jest.MockedFunction
- it's silly but legal) - we should support angle bracket assertions too
function getFixturesRootDir(): string { | ||
return path.join(__dirname, 'fixtures'); | ||
} | ||
|
||
const rootPath = getFixturesRootDir(); | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: require.resolve('@typescript-eslint/parser'), | ||
parserOptions: { | ||
sourceType: 'module', | ||
tsconfigRootDir: rootPath, | ||
project: './tsconfig.json', | ||
}, | ||
}); |
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.
you shouldn't need project
and co since we're not using type information
function getFixturesRootDir(): string { | |
return path.join(__dirname, 'fixtures'); | |
} | |
const rootPath = getFixturesRootDir(); | |
const ruleTester = new RuleTester({ | |
parser: require.resolve('@typescript-eslint/parser'), | |
parserOptions: { | |
sourceType: 'module', | |
tsconfigRootDir: rootPath, | |
project: './tsconfig.json', | |
}, | |
}); | |
const ruleTester = new RuleTester({ | |
parser: require.resolve('@typescript-eslint/parser'), | |
}); |
src/rules/prefer-mocked.ts
Outdated
defaultOptions: [], | ||
create(context) { | ||
return { | ||
'TSAsExpression:has(TSTypeReference > TSQualifiedName:has(Identifier.left[name="jest"]):has(Identifier.right[name="Mock"],Identifier.right[name="MockedFunction"]))'( |
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.
Please don't use complex expressions like this - they're brittle and harder to maintain (in part because TypeScript has no idea what they're doing)
src/rules/prefer-mocked.ts
Outdated
'TSAsExpression:has(TSTypeReference > TSQualifiedName:has(Identifier.left[name="jest"]):has(Identifier.right[name="Mock"],Identifier.right[name="MockedFunction"]))'( | ||
node: ValidatedTsAsExpression, | ||
) { | ||
const fnName = getFnName(node.expression, context.getSourceCode().text); |
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.
Can you see if we can reuse our util here? - I expect you probably can by checking if the parent
is an assertion node
a4c08ed
to
f381f58
Compare
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.
Looks good, just got a few last tweaks - could you also add a test case or two covering multiline shenanigans?
i.e.
(
new Array(100)
.fill(undefined)
.map(x => x.value)
.filter(v => !!v).myProperty as jest.MockedFunction<{
method: () => void;
}>
).mockReturnValue(1);
Also could you check in a test that uses square brackets, just for coverage? i.e. (Obj['foo'] as jest.MockedFunction).mockReturnValue(1);
parser: require.resolve('@typescript-eslint/parser'), | ||
}); | ||
|
||
ruleTester.run('prefer-mocked', rule, { |
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.
nit: generally I'd prefer to not use dedent
for single line calls, and not have a newline between cases
src/rules/prefer-mocked.ts
Outdated
name: __filename, | ||
meta: { | ||
docs: { | ||
description: 'Prefer jest.mocked() over (fn as jest.Mock)', |
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.
description: 'Prefer jest.mocked() over (fn as jest.Mock)', | |
description: 'Prefer `jest.mocked()` over `fn as jest.Mock`', |
src/rules/prefer-mocked.ts
Outdated
description: 'Prefer jest.mocked() over (fn as jest.Mock)', | ||
}, | ||
messages: { | ||
useJestMocked: 'Prefer jest.mocked({{ replacement }})', |
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.
useJestMocked: 'Prefer jest.mocked({{ replacement }})', | |
useJestMocked: 'Prefer `jest.mocked({{ replacement }})`', |
(foo as Mock.jest).mockReturnValue(1); | ||
`, | ||
], | ||
invalid: [ |
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.
can you add the expected data
values for the message in each error?
src/rules/prefer-mocked.ts
Outdated
.text.slice(...followTypeAssertionChain(node.expression).range); | ||
|
||
context.report({ | ||
node: node, |
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.
node: node, | |
node, |
src/rules/prefer-mocked.ts
Outdated
node: node, | ||
messageId: 'useJestMocked', | ||
data: { | ||
replacement: '', |
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.
either we shouldn't have this or it should have a proper value (this'll get picked up by adding it to the tests)
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 think it will be better to remove this parameter, because multi-line code will affect the output of errors
src/rules/prefer-mocked.ts
Outdated
} | ||
|
||
const fnName = context | ||
.getSourceCode() |
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.
please use our getSourceCode
helper here, as getSourceCode
is removed in ESLint v9
src/rules/prefer-mocked.ts
Outdated
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.
nit: I wonder if it might be slightly better to call this prefer-jest-mocked
? 🤔
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.
thanks!
🎉 This PR is included in version 28.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Resolves #1470