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(valid-expect): supporting automatically fixing adding async in some cases #1579

Merged
merged 14 commits into from
Jun 6, 2024

Conversation

y-hsgw
Copy link
Contributor

@y-hsgw y-hsgw commented May 3, 2024

Resolves #1140

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 like a good start, but this isn't quite the right approach to being incremental 😅 - a fix should address the full issue.

What I meant by incremental is landing a fixer for e.g. expect.extend or functions without any nesting or a single expect, then digging into advanced cases like "what if you have a function within your test function?"; but also you don't have to do it like that either - if you want to tackle more in a single PR that's great! based on what you've done here for example, I think you might just need to switch to returning an array of fixes and then this might be landable

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 4, 2024

I misunderstood the concept of incremental improvement.
Instead of partially addressing the issue, I should have aimed to not only add async but also include await where needed.
As you suggested, it seems feasible to address the entire problem within this PR, so I'll give it a shot!

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 5, 2024

I'm encountering a situation where the logic to add 'await' to all expects isn't functioning correctly when there are multiple asynchronous expectations.
If there's a suitable logic you can suggest, I'd appreciate your guidance🙇‍♂️
The relevant test: 6788264#diff-a63b56f41f9d7b260c021d0db0c224fde76950dea886f2d6b97869ca47dd2070R965-R992

@G-Rath
Copy link
Collaborator

G-Rath commented May 5, 2024

@y-hsgw push up your code and I can take a look. That also sounds like a good example of where we might want to be interactive: if you're having trouble with the fixing, consider focusing on having it just bail out instead for now.

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 5, 2024

@G-Rath I pushed. 6788264.

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 6, 2024

I might have misunderstood.
it seems my changes to the test could be incorrect. The official documentation states, 'RuleTester applies only the first fix.' However, I attempted to validate multiple cases simultaneously.

It's likely that the following sequence of fixes will occur:
First: 'async' will be added.
Second: 'await' will be added.

@G-Rath
Copy link
Collaborator

G-Rath commented May 10, 2024

@y-hsgw I think you might be misunderstanding - that relates to when there are conflicts:

When there is a conflict between two fixes (because they apply to the same section of code) RuleTester applies only the first fix.

The fixer works by taking (via the return) an array of fix operations to apply which make up a single fix - if there is only one operation, then you can just return that but you can think about it as being an array of operations at all times.

So when those docs talk about "applying all fixes" and "applies only the first fix" they're referring to the high-level fix aka "a collection of fix operations", which is what you're building here - the issue is right now you're returning two seperate fix operations depending on conditions, when you want to be returning a single array with both operations added conditionally depending on if they're needed for the particular code under lint.

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 10, 2024

@G-Rath
Thank you for the clarification. I had misunderstood and thought there might be conflicts. However, it seems that even with the current logic, async and await are added as shown in the attached video. Would it still be appropriate to return a single array?

2024-05-10.22.04.36.mov

@G-Rath
Copy link
Collaborator

G-Rath commented May 10, 2024

Yes because as the tests show via the output, you're not actually fixing the issue in one pass - it works in your editor because ESLint re-runs rules up to 10 times in a pass to handle overlap between rule fixes; e.g. if we have a fix that outputs single quotes and prettier with double quotes is setup, then the final output of a single run of eslint --fix will produce the right code, because both rules actually got run multiple times.

It should be very trivial to return a single array too :)

@y-hsgw
Copy link
Contributor Author

y-hsgw commented May 11, 2024

@G-Rath
I've refactored to return a single array, but the tests fail when there are multiple expects. Do you have any insights into what might be causing this?
58c4046

@G-Rath
Copy link
Collaborator

G-Rath commented May 11, 2024

@y-hsgw my guess is that it's because you're adding the async in the first fixer, which ends up changing the logic in the second fixer - if I add the async to the function, then both awaits get added correctly.

I would recommend focusing on a single test (you can add only: true to a test case for this) and stick console.logs around every condition and check, then compare what you get with the async present vs when its not.

src/rules/valid-expect.ts Outdated Show resolved Hide resolved
pushPromiseArrayException(finalNode.loc);
}
},
'Program:exit'() {
Copy link
Contributor Author

@y-hsgw y-hsgw May 23, 2024

Choose a reason for hiding this comment

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

@G-Rath

in that case you could push the results into a global array and then do the reporting in Program:exit

I tried making adjustments based on the advice you provided. How does it look? ce98868

@@ -4,7 +4,9 @@
*/

import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils';
import type { RuleFix } from '@typescript-eslint/utils/ts-eslint';
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 pull this type from TSESTree (or it might be TSESLint - either way, you can access it through the @typescript-eslint/utils import)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed in 3b27d09.

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.

awesome stuff! 🎉

@G-Rath G-Rath merged commit 5b9b47e into jest-community:main Jun 6, 2024
36 checks passed
@G-Rath
Copy link
Collaborator

G-Rath commented Jun 6, 2024

hmm

Error: Command failed with exit code 1: git push --tags https://x-access-token:[secure]@github.com/jest-community/eslint-plugin-jest.git HEAD:main
remote: error: GH006: Protected branch update failed for refs/heads/main.        
remote: error: 6 of 6 required status checks are expected.        
To https://github.com/jest-community/eslint-plugin-jest.git
 ! [remote rejected] HEAD -> main (protected branch hook declined)

For some reason the main branch decided to skip our docs job meaning that releasing then failed due to it being a required status check 🤔

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[valid-expect] include fixer for adding missing await
2 participants