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
Assert that async expect
s are awaited or returned - Closes #54, #254
#255
Conversation
@SimenB please take a look to this wip PR. After getting your feedback, I'll update the docs. |
@SimenB addressed :) |
expect
s are awaited or returned - Closes #54expect
s are awaited or returned - Closes #54, #254
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid overall, thanks @yatki! Left a few nits
@SimenB @jeysal first of all, sorry for the delay. I couldn't find time to push this earlier. Now we validate awaiting Please take a look if you experience any problems and let me know what do you think about the latest changes. Cheers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeysal why is this breaking? Seems like a bug fix it it uncovers more cases |
@SimenB expect(Promise.resolve(2)).resolves.toBeDefined(); changed from valid to invalid |
Simen agreed with |
@jeysal I also feel like I'll set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
@SimenB it's turned off by default, so I think we should be safe? Would be cool to turn it on in next major |
@thymikee As far as I see, in recommended settings the rule is enabled, only @SimenB I think it is breaking, we extended the behavior of |
Just to clarify expect(Promise.resolve(2)).resolves.toBeDefined(); |
Sorry, forgot about this! Mind merging in master (I assume a rebase with 20 commits will be painful, and we squash during merge anyways)? Once you've done so, I can run it over some code bases and merge (I know I said so in May... This time I mean it!) |
@SimenB done. I also created messageIds for the messages I wrote! So, I hope this should be fine now. I didn't squash my commits, tho would you like me to do that too? |
Awesome, thanks!
No need, I'll do that when merging. I just left for the day and won't have access to a computer until tomorrow. I'll test this first thing tomorrow though :) |
Finally got around to testing this, this is great! It actually found an error in Jest's own tests (ironically enough when testing async matchers): https://github.com/facebook/jest/blob/ccf90c005232bf9e57ee11023637e31b404a3520/packages/expect/src/__tests__/matchers.test.js#L1220-L1222 However, changes introduced here also crashed in another repo (not expect(foobar); (I pushed it up: ff4d639) It's not a valid (backstory - this |
@SimenB Interesting, the tests were passing. I think the change I made in edit: I got it why they were passing afterwards. |
@SimenB The errors are caused because of the changes I did on I feel like the expectCase function is better as it is now in this PR. It's doing what it should be doing: It's validating if the statement is
Let me know what do you think. |
I agree. Potentially adding a separate util that verifies it's |
Ok then. I will do it that way. Btw, great job on catching that problem. Could cause lots of headaches 😅 🖖🏻 |
I'll be landing #302 btw, which'll probably give you a few conflicts. Shouldn't be too bad, though 🙂 |
@yatki I merged in master, but this now has a failing test. Would you be able to take a look? I'll merge as soon as it's green (feel free to reapply the merge or rebase if you want, but I think I merged correctly) EDIT: I just fixed it, was pretty straight forward. Good thing we added those new tests 😀 |
🎉 This PR is included in version 22.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Wow, that's great :D I immediately updated the plugin to the latest version and seeing it working feels great :) @SimenB thank you very much for the support and constructive feedback. It was a pleasure. Regarding converting it to TS: #333 It's wip at the moment but I'll complete it when i have time this week. |
Acceptance Criteria
valid-expect
rulealwaysAwait
option in plugin configcloses #54,
closes #254