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: add no-confusing-set-time rule #1425

Merged
merged 14 commits into from
Sep 15, 2023

Conversation

eryue0220
Copy link
Contributor

Fix: #1366

@G-Rath G-Rath self-requested a review September 2, 2023 23:57
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.

This is a good start but requires more work - I've done an initial review to give you some pointers, but really your whole implementation needs to be redone to use parseJestFnCall so I've not reviewed that for now.

docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
src/rules/no-confusing-set-timeout.ts Show resolved Hide resolved
src/rules/no-confusing-set-timeout.ts Outdated Show resolved Hide resolved
@eryue0220
Copy link
Contributor Author

eryue0220 commented Sep 5, 2023

Thanks for your excellent guidance and review. I've upgrade according your suggestions.

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.

Have done an initial re-review of the code - its coming together nicely; I'll have a look at the rest during the week, but in the meantime if you have the time to address what I've mentioned currently that'd be great


if (!jestFnCall) return;

if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

If jestFnCall returns non-null it means that we believe the call is something from Jest, even if the type is unknown - so we should be fine to just check if the call is to setTimeout or not; and given that I'd move all that logic into isJsetSetTimeout (i.e. have it take the ParsedJestFnCall and handle checking that the type is jest)

src/rules/no-confusing-set-timeout.ts Show resolved Hide resolved
src/rules/no-confusing-set-timeout.ts Outdated Show resolved Hide resolved
docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
@SimenB SimenB force-pushed the rule/no-confusing-set-timeout branch from 2766207 to b0338c0 Compare September 9, 2023 07:35
Comment on lines 5 to 7
`jest.setTimeout` can be called multiple times anywhere within a single test
file. However, only the last call will have an effect, and it will actually be
invoked before any other jest functions.
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
`jest.setTimeout` can be called multiple times anywhere within a single test
file. However, only the last call will have an effect, and it will actually be
invoked before any other jest functions.
While `jest.setTimeout` can be called multiple times anywhere within a single test
file only the last call before any test functions run will have an effect.

Comment on lines 11 to 12
This rule describes some tricky ways about `jest.setTimeout` that should not
recommend in Jest:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this:

Suggested change
This rule describes some tricky ways about `jest.setTimeout` that should not
recommend in Jest:
This rule checks for several confusing usages of `jest.setTimeout` that looks like it applies to specific tests within the same file, such as:
- being called anywhere other than in global scope
- being called multiple times
- being called after other Jest functions like hooks, `describe`, `test`, or `it`

docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
Comment on lines 4 to 6
function isJestSetTimeout(node: TSESTree.Node) {
return getNodeName(node) === 'jest.setTimeout';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is not correct because it does not account for aliases i.e. this will not get caught:

import { jest as Jest } from '@jest/globals';
{
  Jest.setTimeout(800);
}

While a bit contrived, this is why parseJestFnCall exists and is what you should be checking instead:

function isJestSetTimeout(jestFnCall: ParsedJestFnCall) {
  return (
    jestFnCall.type === 'jest' &&
    jestFnCall.members.length === 1 &&
    isIdentifier(jestFnCall.members[0], 'setTimeout')
  );
}

return;
}

const result = isJestSetTimeout(node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's call this something like thisIsJestSetTimeoutCall - its a bit verbose, but there's no downside and I think its important to be clear given all the "is jest settimeout" names we've got going on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, naming makes my head burn out.

Comment on lines 43 to 45
if (jestFnCall.type !== 'unknown') {
shouldEmitOrderSetTimeout = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

By definition there is no known Jest function call of type unknown - it only exists to gracefully handle if Jest ever did get a new type in older versions of the plugin (since we'd have to release a new version adding support); so you shouldn't worry about it here:

Suggested change
if (jestFnCall.type !== 'unknown') {
shouldEmitOrderSetTimeout = true;
}
shouldEmitOrderSetTimeout = true;

Comment on lines 17 to 19
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope.',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linting messages shouldn't use full stops because they might appear in sentences constructed by IDEs and such:

Suggested change
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope.',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect.',
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope',
multipleSetTimeouts:
'Do not call `jest.setTimeout` multiple times, as only the last call will have an effect',

meta: {
docs: {
category: 'Best Practices',
description: 'Disallow using confusing setTimeout in test',
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: 'Disallow using confusing setTimeout in test',
description: 'Disallow confusing usages of jest.setTimeout',

@eryue0220
Copy link
Contributor Author

Done~ Thanks for your guidances and patience

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.

Just got a small cleanup and a typo, and then I think this is good to go!

return;
}

const thisIsJestSetTimeoutCall = isJestSetTimeout(jestFnCall);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually don't need this variable or your else block at the end because we only do more work if this is a jest.setTimeout - so you can just do:

if (!isJestSetTimeout(jestFnCall)) {
  shouldEmitOrderSetTimeout = true;

  return;
}

// ...

seenJestTimeout = true;

docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Sep 15, 2023

Prettier is still unhappy about something 😅

@G-Rath G-Rath merged commit ff8e482 into jest-community:main Sep 15, 2023
34 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 15, 2023
# [27.3.0](v27.2.3...v27.3.0) (2023-09-15)

### Features

* add `no-confusing-set-time` rule ([#1425](#1425)) ([ff8e482](ff8e482))
@github-actions
Copy link

🎉 This PR is included in version 27.3.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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rule to check placement of jest.setTimeout
3 participants