feat: add no-confusing-set-time rule #1425

eryue0220 · 2023-09-02T09:32:39Z

G-Rath

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

src/rules/__tests__/no-confusing-set-timeout.test.ts

src/rules/no-confusing-set-timeout.ts

eryue0220 · 2023-09-05T01:58:38Z

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

G-Rath

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

G-Rath · 2023-09-05T22:25:54Z

src/rules/no-confusing-set-timeout.ts

+
+        if (!jestFnCall) return;
+
+        if (


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

docs/rules/no-conditional-expect.md

docs/rules/no-confusing-set-timeout.md

G-Rath · 2023-09-09T19:02:43Z

docs/rules/no-confusing-set-timeout.md

+`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.


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.

G-Rath · 2023-09-09T19:22:31Z

docs/rules/no-confusing-set-timeout.md

+This rule describes some tricky ways about `jest.setTimeout` that should not
+recommend in Jest:


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

G-Rath · 2023-09-09T19:42:14Z

src/rules/no-confusing-set-timeout.ts

+function isJestSetTimeout(node: TSESTree.Node) {
+  return getNodeName(node) === 'jest.setTimeout';
+}


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

G-Rath · 2023-09-09T19:43:02Z

src/rules/no-confusing-set-timeout.ts

+          return;
+        }
+
+        const result = isJestSetTimeout(node);


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

Yeah, naming makes my head burn out.

G-Rath · 2023-09-09T19:53:24Z

src/rules/no-confusing-set-timeout.ts

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


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;

G-Rath · 2023-09-09T19:54:04Z

src/rules/no-confusing-set-timeout.ts

+      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.',


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',

G-Rath · 2023-09-09T19:54:20Z

src/rules/no-confusing-set-timeout.ts

+  meta: {
+    docs: {
+      category: 'Best Practices',
+      description: 'Disallow using confusing setTimeout in test',


Suggested change

description: 'Disallow using confusing setTimeout in test',

description: 'Disallow confusing usages of jest.setTimeout',

eryue0220 · 2023-09-11T15:01:45Z

Done~ Thanks for your guidances and patience

G-Rath

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

G-Rath · 2023-09-11T20:04:04Z

src/rules/no-confusing-set-timeout.ts

+          return;
+        }
+
+        const thisIsJestSetTimeoutCall = isJestSetTimeout(jestFnCall);


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

SimenB · 2023-09-15T14:42:42Z

Prettier is still unhappy about something 😅

# [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 · 2023-09-15T22:46:05Z

🎉 This PR is included in version 27.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

eryue0220 added 2 commits September 2, 2023 17:28

feat: add no-confusing-set-time rule

b0d4ff4

Merge branch 'main' into rule/no-confusing-set-timeout

a1eec07

G-Rath self-requested a review September 2, 2023 23:57

G-Rath requested changes Sep 3, 2023

View reviewed changes

fix: cr problems

0a98694

G-Rath requested changes Sep 5, 2023

View reviewed changes

eryue0220 added 5 commits September 7, 2023 15:19

fix: ci

7e2ce4b

Merge branch 'main' into rule/no-confusing-set-timeout

b21df04

feat: reactor rule

59da5f3

chore: upgrade

25b2d93

Merge branch 'main' into rule/no-confusing-set-timeout

a2e9080

SimenB reviewed Sep 9, 2023

View reviewed changes

docs/rules/no-conditional-expect.md Outdated Show resolved Hide resolved

SimenB reviewed Sep 9, 2023

View reviewed changes

docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved

docs/rules/no-confusing-set-timeout.md Outdated Show resolved Hide resolved

chore: fix lint

b0338c0

SimenB force-pushed the rule/no-confusing-set-timeout branch from 2766207 to b0338c0 Compare September 9, 2023 07:35

G-Rath requested changes Sep 9, 2023

View reviewed changes

fix: ci problems

c39b755

G-Rath requested changes Sep 11, 2023

View reviewed changes

eryue0220 added 2 commits September 12, 2023 06:20

fix: cr problems

40fa046

fix: ci

80f525e

G-Rath added 2 commits September 16, 2023 10:33

Merge branch 'main' into rule/no-confusing-set-timeout

382ddbb

docs: regenerate

c5e73ee

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

chore(release): 27.3.0 [skip ci]

bc4eae4

# [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 bot added the released label Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add no-confusing-set-time rule #1425

feat: add no-confusing-set-time rule #1425

eryue0220 commented Sep 2, 2023

G-Rath left a comment

eryue0220 commented Sep 5, 2023 •

edited

Loading

G-Rath left a comment

G-Rath Sep 5, 2023

G-Rath Sep 9, 2023

G-Rath Sep 9, 2023

G-Rath Sep 9, 2023

G-Rath Sep 9, 2023

eryue0220 Sep 11, 2023

G-Rath Sep 9, 2023

G-Rath Sep 9, 2023

G-Rath Sep 9, 2023

eryue0220 commented Sep 11, 2023

G-Rath left a comment

G-Rath Sep 11, 2023

SimenB commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

		This rule describes some tricky ways about `jest.setTimeout` that should not
		recommend in Jest:

-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`

	description: 'Disallow using confusing setTimeout in test',
	description: 'Disallow confusing usages of jest.setTimeout',

feat: add no-confusing-set-time rule #1425

feat: add no-confusing-set-time rule #1425

Conversation

eryue0220 commented Sep 2, 2023

G-Rath left a comment

Choose a reason for hiding this comment

eryue0220 commented Sep 5, 2023 • edited Loading

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eryue0220 commented Sep 11, 2023

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

eryue0220 commented Sep 5, 2023 •

edited

Loading