jest-community / eslint-plugin-jest

ESLint plugin for Jest
MIT License
1.12k stars 228 forks source link

feat(valid-expect): supporting automatically fixing adding async in some cases #1579

Closed y-hsgw closed 1 month ago

y-hsgw commented 2 months ago

Resolves #1140

y-hsgw commented 2 months ago

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 commented 2 months ago

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: https://github.com/jest-community/eslint-plugin-jest/pull/1579/commits/6788264f88ebbc9c83771ec32d42973e192ff97d#diff-a63b56f41f9d7b260c021d0db0c224fde76950dea886f2d6b97869ca47dd2070R965-R992

G-Rath commented 2 months ago

@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 commented 2 months ago

@G-Rath I pushed. 6788264.

y-hsgw commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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?

https://github.com/jest-community/eslint-plugin-jest/assets/49516827/9934cc2d-8bf6-4622-b5b8-1d36a1132a29

G-Rath commented 2 months ago

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 commented 2 months ago

@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 commented 1 month ago

@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.

G-Rath commented 1 month ago

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] commented 1 month ago

:tada: This PR is included in version 28.6.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: