feat: resolve `expect` based on scope #1173

G-Rath · 2022-07-24T20:04:44Z

This switches our handling of expect over to match the rest of our we now resolve jest functions, meaning it correctly respects @jest/globals & the related patterns that come with that, done through parseJestFnCall.

This required a lot more work than the others as currently we handle expect calls from the expect(...) call itself whereas for e.g. test we do it at the "top" call expression (test.only(...)) - inverting this for expect had some interesting impacts, the biggest of which is to valid-expect because now the act of parsing effectively enforces a "valid" expect statement, which now is effectively a wrapper for exposing the "reason" that parseJestFnCall internally now returns (which is to keep valid-expect usable in the first place, and is only exposed with parseJestFnCallWithReason).

Elsewhere some places have become easier (unbound-method & no-restricted-matchers especially) but it's also gotten more technical, so overall I think there's no net loss or gain (which is fine).

This does come with a minor performance hit since we're doing a decent amount more work - as a basic test I checked the times of running jest/all on all file types in the jest monorepo, and we're looking at about an extra 10 seconds; also importantly parseJestFnCall has a significant enough overhead that we need to be careful about calling it multiple times unless we really have to (I was able to clearly optimize the time of no-standalone-expect & prefer-expect-assertions this way).

While I think the current performance hit doesn't bother me (it's still less than 45 seconds on average, and when you include other rules that gets to 60s+) but that second point does as it's something we have to watch out for - luckily it looks like we can easily fix this by just chucking in a cache, which brings the times down to 10 seconds; I'm planning to land that in another PR to keep things clean (+ we'd have a cacheless version we can baseline against).

I'm opening this as a draft because I want to do some cleanup before it's merged and there are edge-cases to think about (especially in valid-expect), but it should be reviewable already. I focused first on just replacing parseExpectCall with the low level API, to avoid premature optimizing - I plan to go back through the rules and identity common usage/patterns that we can expose on the parsed call, such as accessing the arguments for expect and checking modifiers nodes.

As expected this has also identified a few edge-case bugs which I might try and land ahead of time to make things a bit easier.

I'm also going to run the smoke test against this branch.

G-Rath · 2022-07-29T23:20:09Z

I toyed around with adding some helpers to the parsed jest call e.g. jestFnCall.hasMember('each') which were neat but I've not included them for now because our rules are pretty stable these days and they didn't reduce the overall compiled size so don't think they're worth landing right now.

I might add them in the future if we get some more rules that would benefit from them.

… checks

…or rules

…pect` checks

SimenB

exciting stuff!

SimenB · 2022-07-30T10:44:49Z

src/rules/__tests__/no-restricted-matchers.test.ts

@@ -70,7 +70,7 @@ ruleTester.run('no-restricted-matchers', rule, {
      ],
    },
    {
-      code: 'expect(a).not',
+      code: 'expect(a).not[x]()',


shouldn't we keep the old test?

The new parser doesn't consider the old test to be a valid expect call, and we've already got expect(a).rejects as a valid test which covers this change 🤷

SimenB · 2022-07-30T10:45:09Z

src/rules/__tests__/no-standalone-expect.test.ts

@@ -20,7 +20,6 @@ ruleTester.run('no-standalone-expect', rule, {
    'it("an it", () => expect(1).toBe(1))',
    'const func = function(){ expect(1).toBe(1); };',
    'const func = () => expect(1).toBe(1);',
-    'expect.hasAssertions()',


This has become an error, since it's a standalone expect.

SimenB · 2022-07-30T10:45:42Z

src/rules/__tests__/no-standalone-expect.test.ts

-      code: "(() => {})('testing', () => expect(true))",
-      errors: [{ endColumn: 41, column: 29, messageId: 'unexpectedExpect' }],
+      code: "(() => {})('testing', () => expect(true).toBe(false))",
+      errors: [{ column: 29, messageId: 'unexpectedExpect' }],


why no endColumn

Eh I typically don't include it as I don't think its super valuable since we can't control how the results are used in tools & editors so it just creates more work for us without much gain, but yeah I should include it here for consistency since the other tests have it 😅

SimenB · 2022-07-30T10:46:34Z

src/rules/__tests__/prefer-equality-matcher.test.ts

+    'expect.hasAssertions',
+    'expect.hasAssertions()',
+    'expect.assertions(1)',
+    'expect(true).toBe(...true)',


looks like a syntax error to me 😅

# [26.8.0](v26.7.0...v26.8.0) (2022-08-07) ### Features * resolve `expect` based on scope ([#1173](#1173)) ([aa4be21](aa4be21))

github-actions · 2022-08-07T18:06:18Z

🎉 This PR is included in version 26.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

G-Rath requested a review from SimenB July 24, 2022 20:04

G-Rath force-pushed the parse-expect3 branch from c37dce9 to 4d6398e Compare July 25, 2022 20:09

This was referenced Jul 29, 2022

chore: change rules to always report on matcher even when modifier is present #1177

Merged

refactor(prefer-called-with): rename name data property to matcherName #1178

Merged

G-Rath force-pushed the parse-expect3 branch 6 times, most recently from c15b40a to 407bc9c Compare July 29, 2022 22:55

G-Rath mentioned this pull request Jul 29, 2022

refactor(valid-expect): eliminate duplicate check #1179

Merged

G-Rath marked this pull request as ready for review July 29, 2022 23:17

G-Rath force-pushed the parse-expect3 branch from 7f82979 to 3dce894 Compare July 29, 2022 23:32

G-Rath added 15 commits July 30, 2022 12:04

feat: parse expect with new jest fn call parser

d94889c

feat: switch rules over to using new jest fn call parser for expect…

13ad62c

… checks

fix: restructure expect parsing to include additional information f…

be6c9ba

…or rules

feat: wip

bf3f040

feat: wip 2

db349f8

feat: switch more rules over to using new jest fn call parser for `ex…

1925b65

…pect` checks

feat: wip 3

f7f30cd

feat: switch more rules over to using new jest fn call parser for `ex…

e97533e

…pect` checks

feat: remove parseExpectCall and friends

6f82836

refactor: deduplicate findTopMostCallExpression utility

08d0615

refactor: optimize some code for full coverage & cleanup wip comments

4d616ca

perf: optimize no-standalone-expect & prefer-expect-assertions

f258c58

refactor: deduplicate isBooleanLiteral & getFirstMatcherArg utils

b8fd7db

refactor: include matcher and modifiers in parsed expect fn call

7d4d4b6

test: add some cases that has aliased expect imports

016ba2b

G-Rath force-pushed the parse-expect3 branch from 3dce894 to 016ba2b Compare July 30, 2022 00:04

SimenB approved these changes Jul 30, 2022

View reviewed changes

test(no-standalone-expect): add endColumn to new specs

fc73465

SimenB merged commit aa4be21 into main Aug 7, 2022

SimenB deleted the parse-expect3 branch August 7, 2022 18:02

github-actions bot pushed a commit that referenced this pull request Aug 7, 2022

chore(release): 26.8.0 [skip ci]

d1ee640

# [26.8.0](v26.7.0...v26.8.0) (2022-08-07) ### Features * resolve `expect` based on scope ([#1173](#1173)) ([aa4be21](aa4be21))

github-actions bot added the released label Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve `expect` based on scope #1173

feat: resolve `expect` based on scope #1173

G-Rath commented Jul 24, 2022

G-Rath commented Jul 29, 2022

SimenB left a comment

SimenB Jul 30, 2022

G-Rath Aug 6, 2022

SimenB Jul 30, 2022

G-Rath Aug 6, 2022

SimenB Jul 30, 2022

G-Rath Aug 6, 2022

SimenB Jul 30, 2022

github-actions bot commented Aug 7, 2022

feat: resolve expect based on scope #1173

feat: resolve expect based on scope #1173

Conversation

G-Rath commented Jul 24, 2022

G-Rath commented Jul 29, 2022

SimenB left a comment

Choose a reason for hiding this comment

SimenB Jul 30, 2022

Choose a reason for hiding this comment

G-Rath Aug 6, 2022

Choose a reason for hiding this comment

SimenB Jul 30, 2022

Choose a reason for hiding this comment

G-Rath Aug 6, 2022

Choose a reason for hiding this comment

SimenB Jul 30, 2022

Choose a reason for hiding this comment

G-Rath Aug 6, 2022

Choose a reason for hiding this comment

SimenB Jul 30, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 7, 2022

feat: resolve `expect` based on scope #1173

feat: resolve `expect` based on scope #1173