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

feat(valid-expect): warn on await expect() with no assertions #496

Merged
merged 1 commit into from Dec 28, 2019

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Dec 18, 2019

Makes the following an error under the valid-expect rule:

await expect('something'); // No assertion was called on expect().

Rationale

As this plugin already has a rule for expect-with-no-assertions, I thought I'd add a similar one for await expect.

Speaking from recent experience, the simple mistake of awaiting the wrong thing (await expect('something') instead of await '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).

Copy link
Collaborator

@G-Rath G-Rath left a 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)

src/rules/valid-expect.ts Outdated Show resolved Hide resolved
src/rules/valid-expect.ts Outdated Show resolved Hide resolved
src/rules/valid-expect.ts Outdated Show resolved Hide resolved
@motiz88
Copy link
Contributor Author

motiz88 commented Dec 19, 2019

It's interesting that it doesn't already pick this up

There might be a case for inverting the check on the parent node altogether. What if we made it so expect() had to be the object of a MemberExpression, and anything else would error with noAssertions?

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

@G-Rath
Copy link
Collaborator

G-Rath commented Dec 19, 2019

Is the following a common/allowed pattern at all? (And if so, how do we feel about forbidding it?)
What if we made it so expect() had to be the object of a MemberExpression, and anything else would error with noAssertions?

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.

@G-Rath G-Rath merged commit 19798dd into jest-community:master Dec 28, 2019
github-actions bot pushed a commit that referenced this pull request Dec 28, 2019
# [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))
@github-actions
Copy link

🎉 This PR is included in version 23.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants