Skip to content
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

Merged
merged 2 commits into from
Jun 6, 2024
Merged

Conversation

MillerSvt
Copy link
Contributor

@MillerSvt MillerSvt commented May 30, 2024

Resolves #1470

@MillerSvt MillerSvt changed the title feat(prefer-mocked): add new rule (#1470) feat(prefer-mocked): add new rule May 30, 2024
@SimenB SimenB requested a review from G-Rath May 30, 2024 15:30
@MillerSvt
Copy link
Contributor Author

error - 2024-05-30 16:03:56,619 -- Commit creating failed: {"detail":"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 2006 seconds."}
Error: Codecov: Failed to properly create commit: The process '/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov' failed with exit code 1

It looks like CI is broken.

Copy link
Collaborator

@G-Rath G-Rath left a 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 and as unknown as jest.MockedFunction as string as jest.MockedFunction - it's silly but legal)
  • we should support angle bracket assertions too

Comment on lines 6 to 7
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',
},
});
Copy link
Collaborator

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

Suggested change
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'),
});

defaultOptions: [],
create(context) {
return {
'TSAsExpression:has(TSTypeReference > TSQualifiedName:has(Identifier.left[name="jest"]):has(Identifier.right[name="Mock"],Identifier.right[name="MockedFunction"]))'(
Copy link
Collaborator

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)

'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);
Copy link
Collaborator

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

Copy link
Collaborator

@G-Rath G-Rath left a 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, {
Copy link
Collaborator

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

name: __filename,
meta: {
docs: {
description: 'Prefer jest.mocked() over (fn as jest.Mock)',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: 'Prefer jest.mocked() over (fn as jest.Mock)',
description: 'Prefer `jest.mocked()` over `fn as jest.Mock`',

description: 'Prefer jest.mocked() over (fn as jest.Mock)',
},
messages: {
useJestMocked: 'Prefer jest.mocked({{ replacement }})',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
useJestMocked: 'Prefer jest.mocked({{ replacement }})',
useJestMocked: 'Prefer `jest.mocked({{ replacement }})`',

(foo as Mock.jest).mockReturnValue(1);
`,
],
invalid: [
Copy link
Collaborator

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?

.text.slice(...followTypeAssertionChain(node.expression).range);

context.report({
node: node,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
node: node,
node,

node: node,
messageId: 'useJestMocked',
data: {
replacement: '',
Copy link
Collaborator

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)

Copy link
Contributor Author

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

}

const fnName = context
.getSourceCode()
Copy link
Collaborator

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

Copy link
Collaborator

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? 🤔

@G-Rath G-Rath added the new rule label Jun 6, 2024
@G-Rath G-Rath changed the title feat(prefer-mocked): add new rule feat(prefer-jest-mocked): add new rule Jun 6, 2024
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

thanks!

@G-Rath G-Rath merged commit 4b6a4f2 into jest-community:main Jun 6, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 6, 2024
# [28.6.0](v28.5.0...v28.6.0) (2024-06-06)

### Features

* **prefer-jest-mocked:** add new rule ([#1599](#1599)) ([4b6a4f2](4b6a4f2))
* **valid-expect:** supporting automatically fixing adding async in some cases ([#1579](#1579)) ([5b9b47e](5b9b47e))
Copy link

github-actions bot commented Jun 6, 2024

🎉 This PR is included in version 28.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new-rule] Force to use jest.mocked(fn) instead of fn as jest.Mock
2 participants