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(rules): no-try-expect #331

Merged
merged 4 commits into from Jul 21, 2019
Merged

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Jul 21, 2019

This rule prevents the use of expect inside catch blocks.

The following patterns are warnings:

it('foo', () => {
 try {
   foo(); // `foo` may be refactored to not throw exceptions, yet still appears to be tested here.
 } catch (err) {
   expect(err).toMatch(/foo error/);
 }
});
it('bar', async () => {
 try {
   await foo();
 } catch (err) {
   expect(err).toMatch(/foo error/);
 }
});

The following patterns are not warnings:

it('foo', () => {
 expect(() => foo()).toThrow(/foo error/);
});
it('bar', async () => {
 await expect(fooPromise).rejects.toThrow(/foo error/);
});

Related #295

src/rules/__tests__/no-try-expect.test.js Outdated Show resolved Hide resolved
src/rules/__tests__/no-try-expect.test.js Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is a great rule! 🙂

docs/rules/no-try-expect.md Outdated Show resolved Hide resolved
docs/rules/no-try-expect.md Show resolved Hide resolved
src/rules/__tests__/no-try-expect.test.js Outdated Show resolved Hide resolved
docs/rules/no-try-expect.md Outdated Show resolved Hide resolved
## Rule Details

Expectations inside a `catch` block can be silently skipped. While Jest provides
an `expect.assertions(number)` helper, it might be cumbersome to add this to
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if the rule should check for these and pass if the test uses it, for the cases where one might actually need complicated logic in a catch

Copy link
Member

Choose a reason for hiding this comment

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

in that case I think an eslint-disable-next-line comments is better rather than try to make the rule too clever.

Copy link
Member

Choose a reason for hiding this comment

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

Not if you eslint-disable without adding expect.assertions - the lint error could suggest adding expect.assertions to get rid of the warning and save someone from mindlessly disabling it

Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
@SimenB SimenB merged commit f09bc99 into jest-community:master Jul 21, 2019
@SimenB
Copy link
Member

SimenB commented Jul 21, 2019

🎉 This PR is included in version 22.13.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

4 participants