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

[y-hsgw]

Resolves #1140

[G-Rath]

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]

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]

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]

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

@G-Rath I pushed. 6788264.

[y-hsgw]

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]

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

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

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]

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

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

[y-hsgw]

@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

[G-Rath]

can you pull this type from TSESTree (or it might be TSESLint - either way, you can access it through the @typescript-eslint/utils import)

[y-hsgw]

Thanks! Fixed in 3b27d09.

[G-Rath]

awesome stuff! 🎉

[G-Rath]

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]

🎉 This PR is included in version 28.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀