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

Assert that async expects are awaited or returned - Closes #54, #254 #255

Merged
merged 27 commits into from Jul 20, 2019
Merged

Assert that async expects are awaited or returned - Closes #54, #254 #255

merged 27 commits into from Jul 20, 2019

Conversation

yatki
Copy link
Contributor

@yatki yatki commented May 9, 2019

Acceptance Criteria

  • Improve valid-expect rule
  • Add test coverage
  • Update docs
  • Enable alwaysAwait option in plugin config

closes #54,
closes #254

@yatki
Copy link
Contributor Author

yatki commented May 9, 2019

@SimenB please take a look to this wip PR. After getting your feedback, I'll update the docs.

index.js Outdated Show resolved Hide resolved
rules/valid-expect.js Outdated Show resolved Hide resolved
@yatki
Copy link
Contributor Author

yatki commented May 9, 2019

@SimenB addressed :)

@yatki yatki changed the title Assert that async expects are awaited or returned - Closes #54 Assert that async expects are awaited or returned - Closes #54, #254 May 9, 2019
Copy link
Member

@jeysal jeysal left a 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

rules/valid-expect.js Outdated Show resolved Hide resolved
rules/valid-expect.js Outdated Show resolved Hide resolved
rules/util.js Outdated Show resolved Hide resolved
@yatki
Copy link
Contributor Author

yatki commented May 18, 2019

@SimenB @jeysal first of all, sorry for the delay. I couldn't find time to push this earlier.

Now we validate awaiting Promise.x(expect) and Promise.x([expect1, expect2]) usages. I also introduced ignoreInPromise option to disable reporting errors when async asserts are in promise methods. I also tried to make sure nothing breaks when we type.

Please take a look if you experience any problems and let me know what do you think about the latest changes.

Cheers.

Copy link
Member

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

@yatki don't worry about the delay, we all have limited time available ;)
@SimenB maybe also take a look again, a lot was rewritten

We should remember to merge it as breaking BTW

rules/__tests__/valid-expect.test.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented May 19, 2019

@jeysal why is this breaking? Seems like a bug fix it it uncovers more cases

@jeysal
Copy link
Member

jeysal commented May 19, 2019

@SimenB
Because of alwaysAwait: true in the preset (although that might be changed now) and because

expect(Promise.resolve(2)).resolves.toBeDefined();

changed from valid to invalid

@yatki yatki marked this pull request as ready for review May 19, 2019 14:21
@yatki
Copy link
Contributor Author

yatki commented May 21, 2019

@SimenB @jeysal just wanted to follow up, if you were able to discuss & decide what to do about the remaining topics? So I can update the docs & conclude the PR this week when I have time ^^.

  • Renaming the options
  • Default values of the options

Cheers.

@jeysal
Copy link
Member

jeysal commented May 21, 2019

Simen agreed with alwaysAwait defaulting to false, so that's safe. Also doubt he'd be against the rename. Looking at your third really makes me want to default allowPromiseMethods to false, since that is quite a severe issue to miss (in fact, I'm not sure why you'd want to enable it at all if the behavior without it already properly considers await/return).

@yatki
Copy link
Contributor Author

yatki commented May 22, 2019

@jeysal I also feel like allowPromiseMethods sounds better. And I agree, I also wouldn't never enable allowPromiseMethods but since supporting Promise methods was an additional feature, i just wanted to provide a way to easily disable it just in case somebody wouldn't like to use it.

I'll set alwaysAwait to false by default, rename the option ignoreInPromise to allowPromiseMethods and set it to false by default and update the docs asap.

@yatki
Copy link
Contributor Author

yatki commented May 26, 2019

@jeysal @SimenB changes were addressed. I also updated the docs. Please feel free to edit the docs, if you want to add or correct something.

Looking forward for your feedback and to use this rule with the plugin 🖖

Copy link
Member

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Awesome!

docs/rules/valid-expect.md Outdated Show resolved Hide resolved
src/rules/valid-expect.js Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented May 28, 2019

Thanks! I'll run this over some code bases later today and merge if all is good :)

@jeysal @thymikee do you agree this is not breaking?

@thymikee
Copy link
Member

@SimenB it's turned off by default, so I think we should be safe? Would be cool to turn it on in next major

@yatki
Copy link
Contributor Author

yatki commented May 28, 2019

@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 alwaysAwait option is disabled but the behaviour of the valid-expect changed anyways.

@SimenB I think it is breaking, we extended the behavior of valid-expect. We were not triggering warnings before on misused async assertions but now in lots of projects, linting step on CI will fail because of new checks in the rule. So actually this is not backwards compatible?!

@yatki
Copy link
Contributor Author

yatki commented May 28, 2019

Just to clarify alwaysAwait only enforces to use await instead of returning Promises. even alwaysAwait is false, the rule still would trigger warning on

expect(Promise.resolve(2)).resolves.toBeDefined();

@yatki
Copy link
Contributor Author

yatki commented Jul 15, 2019

@SimenB @jeysal is there any update on this PR? Can you estimate when this can be released? Is it going to be released? Cheers,

@SimenB
Copy link
Member

SimenB commented Jul 15, 2019

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!)

@yatki
Copy link
Contributor Author

yatki commented Jul 15, 2019

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

@SimenB
Copy link
Member

SimenB commented Jul 15, 2019

Awesome, thanks!

I didn't squash my commits, tho would you like me to do that too?

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 :)

@SimenB
Copy link
Member

SimenB commented Jul 16, 2019

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 valid-expect though - no-alias-methods). Failing snippet:

expect(foobar);

(I pushed it up: ff4d639)

It's not a valid expect call (there's no expectation), but it explodes with TypeError: Cannot read property 'name' of undefined

(backstory - this expect is from cypress so we've turned off valid-expect for these files, which is why it slipped through)

@yatki
Copy link
Contributor Author

yatki commented Jul 16, 2019

@SimenB Interesting, the tests were passing. I think the change I made in expectCase helper function is causing that error. I'm looking into it.

edit: I got it why they were passing afterwards.

@yatki
Copy link
Contributor Author

yatki commented Jul 16, 2019

@SimenB The errors are caused because of the changes I did on expectCase. I'm going to fix it but I don't have time for today. I'll do it tomorrow probably. Something to think on, would you prefer me to revert the changes on expectCase or fix the rules that use expectCase.

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 expect() statement. Checking if that node has arguments or properties should be done afterwards depending on the logic of the rule. So, I suggest fixing the rules that's using expectCase:

  • src\rules\no-alias-methods.js
  • src\rules\no-truthy-falsy.js
  • src\rules\prefer-called-with.js
  • src\rules\prefer-strict-equal.js
  • src\rules\prefer-to-contain.js
  • src\rules\prefer-to-have-length.js
  • src\rules\require-tothrow-message.js

Let me know what do you think.

@SimenB
Copy link
Member

SimenB commented Jul 16, 2019

I agree. Potentially adding a separate util that verifies it's expect and has matchers so we don't have to duplicate it across rules

@yatki
Copy link
Contributor Author

yatki commented Jul 16, 2019

Ok then. I will do it that way. Btw, great job on catching that problem. Could cause lots of headaches 😅 🖖🏻

@SimenB
Copy link
Member

SimenB commented Jul 16, 2019

I'll be landing #302 btw, which'll probably give you a few conflicts. Shouldn't be too bad, though 🙂

@SimenB
Copy link
Member

SimenB commented Jul 20, 2019

@yatki I merged in master, but this now has a failing test. Would you be able to take a look?

image

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 😀

@SimenB SimenB merged commit 7ae98f5 into jest-community:master Jul 20, 2019
@SimenB SimenB mentioned this pull request Jul 20, 2019
@SimenB
Copy link
Member

SimenB commented Jul 20, 2019

Finally! 🎉 Thank you for a mind-boggling awesome contribution @yatki!

If you feel up for it, I'd love your help converting this rule to TypeScript (#256) 🙂 I feel like you know this rule best by now 😛

@SimenB
Copy link
Member

SimenB commented Jul 20, 2019

🎉 This PR is included in version 22.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@yatki
Copy link
Contributor Author

yatki commented Jul 22, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants