-
Notifications
You must be signed in to change notification settings - Fork 242
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): warn on await expect()
with no assertions
#496
Conversation
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 good to me!
It's interesting that it doesn't already pick this up since I would have figured it'd be reported, but apparently not 🤷♀
I've left two comments that are not blocking, but would be cool if you wouldn't mind addressing 🙂
(I'll merge in a bit, just to give you some time to see this)
8155a3d
to
07f0c76
Compare
07f0c76
to
d737cdf
Compare
There might be a case for inverting the check on the parent node altogether. What if we made it so Is the following a common/allowed pattern at all? (And if so, how do we feel about forbidding it?) const ex = expect(fooBar());
ex.toBe(true);
ex.not.toBe(false);
// etc This doesn't have to block this PR. Just curious :) |
That's an interesting thought, which I will think on over the holidays. When I converted this plugin to TypeScript, I looked into exactly that sort of thing, and my conclusion was that there is a fine balance to try and strike w/ AST handling, as so to support generally as much as possible w/o being too open ended that you need to do a whole bunch of checks anytime you want to sneeze. I don't think that change would be a big win for us, as it's not something we've got lots of bugs on (that I know of?), but I'll have a think on it b/c it is an interesting thought. |
# [23.2.0](v23.1.1...v23.2.0) (2019-12-28) ### Features * **valid-expect:** warn on `await expect()` with no assertions ([#496](#496)) ([19798dd](19798dd))
🎉 This PR is included in version 23.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Makes the following an error under the
valid-expect
rule:Rationale
As this plugin already has a rule for
expect
-with-no-assertions, I thought I'd add a similar one forawait expect
.Speaking from recent experience, the simple mistake of awaiting the wrong thing (
await expect('something')
instead ofawait 'something'
) can be a pain to track down and fix, as its effects are likely to be felt far from the line containing the actual mistake (which itself will never throw at runtime).