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: resolve expect based on scope #1173

Merged
merged 16 commits into from
Aug 7, 2022
Merged

feat: resolve expect based on scope #1173

merged 16 commits into from
Aug 7, 2022

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Jul 24, 2022

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
Copy link
Collaborator Author

G-Rath commented Jul 29, 2022

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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

exciting stuff!

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

Choose a reason for hiding this comment

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

shouldn't we keep the old test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 🤷

@@ -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()',
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

why no endColumn

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 😅

'expect.hasAssertions',
'expect.hasAssertions()',
'expect.assertions(1)',
'expect(true).toBe(...true)',
Copy link
Member

Choose a reason for hiding this comment

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

looks like a syntax error to me 😅

@SimenB SimenB merged commit aa4be21 into main Aug 7, 2022
@SimenB 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
# [26.8.0](v26.7.0...v26.8.0) (2022-08-07)

### Features

* resolve `expect` based on scope ([#1173](#1173)) ([aa4be21](aa4be21))
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version 26.8.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.

2 participants