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
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -127,6 +127,7 @@ installations requiring long-term consistency.
| [no-test-prefixes][] | Disallow using `f` & `x` prefixes to define focused/skipped tests | ![recommended][] | ![fixable-green][] |
| [no-test-return-statement][] | Disallow explicitly returning from tests | | |
| [no-truthy-falsy][] | Disallow using `toBeTruthy()` & `toBeFalsy()` | | |
| [no-try-expect][] | Prevent `catch` assertions in tests | | |
| [prefer-expect-assertions][] | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | |
| [prefer-spy-on][] | Suggest using `jest.spyOn()` | | ![fixable-green][] |
| [prefer-strict-equal][] | Suggest using `toStrictEqual()` | | ![fixable-green][] |
Expand Down Expand Up @@ -177,6 +178,7 @@ https://github.com/dangreenisrael/eslint-plugin-jest-formatting
[no-test-prefixes]: docs/rules/no-test-prefixes.md
[no-test-return-statement]: docs/rules/no-test-return-statement.md
[no-truthy-falsy]: docs/rules/no-truthy-falsy.md
[no-try-expect]: docs/rules/no-try-expect.md
[prefer-called-with]: docs/rules/prefer-called-with.md
[prefer-expect-assertions]: docs/rules/prefer-expect-assertions.md
[prefer-spy-on]: docs/rules/prefer-spy-on.md
Expand Down
56 changes: 56 additions & 0 deletions docs/rules/no-try-expect.md
@@ -0,0 +1,56 @@
# Prevent catch assertions in tests (no-try-expect)

This rule prevents the use of `expect` inside `catch` blocks.

## 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

every single test. Using `toThrow` concisely guarantees that an exception was
thrown, and that its contents match expectations.

The following patterns are warnings:

```js
cartogram marked this conversation as resolved.
Show resolved Hide resolved
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/);
}
});

it('baz', async () => {
try {
await foo();
} catch (err) {
expect(err).toMatchObject({ code: 'MODULE_NOT_FOUND' });
}
});
```

The following patterns are not warnings:

```js
it('foo', () => {
expect(() => foo()).toThrow(/foo error/);
});

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

it('baz', async () => {
expect(() => foo()).toThrow(
cartogram marked this conversation as resolved.
Show resolved Hide resolved
expect.objectContaining({ code: 'MODULE_NOT_FOUND' }),
);
});
```
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.js
Expand Up @@ -3,7 +3,7 @@ import { resolve } from 'path';
import { rules } from '../';

const ruleNames = Object.keys(rules);
const numberOfRules = 35;
const numberOfRules = 36;

describe('rules', () => {
it('should have a corresponding doc for each rule', () => {
Expand Down
62 changes: 62 additions & 0 deletions src/rules/__tests__/no-try-expect.test.js
@@ -0,0 +1,62 @@
import { RuleTester } from 'eslint';
import rule from '../no-try-expect';

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 2019,
},
});

ruleTester.run('no-try-catch', rule, {
valid: [
`it('foo', () => {
expect('foo').toEqual('foo');
})`,
`it('foo', () => {
expect('bar').toEqual('bar');
});
try {

} catch {
expect('foo').toEqual('foo');
}`,
`it.skip('foo');
try {

} catch {
expect('foo').toEqual('foo');
}`,
],
invalid: [
{
code: `it('foo', () => {
try {

} catch (err) {
expect(err).toMatch('Error');
}
})`,
errors: [
{
messageId: 'noTryExpect',
},
],
},
{
code: `it('foo', async () => {
await wrapper('production', async () => {
try {

} catch (err) {
expect(err).toMatch('Error');
}
})
})`,
errors: [
{
messageId: 'noTryExpect',
},
],
},
],
});
53 changes: 53 additions & 0 deletions src/rules/no-try-expect.js
@@ -0,0 +1,53 @@
import { getDocsUrl, isTestCase } from './util';

export default {
meta: {
docs: {
description: 'Prefer using toThrow for exception tests',
uri: getDocsUrl(__filename),
},
messages: {
noTryExpect: [
'Tests should use Jest‘s exception helpers.',
'Use "expect(() => yourFunction()).toThrow()" for synchronous tests,',
'or "await expect(yourFunction()).rejects.toThrow()" for async tests',
].join(' '),
},
},
create(context) {
let isTest = false;
let catchDepth = 0;

function isThrowExpectCall(node) {
return catchDepth > 0 && node.callee.name === 'expect';
}

return {
CallExpression(node) {
if (isTestCase(node)) {
isTest = true;
} else if (isTest && isThrowExpectCall(node)) {
context.report({
messageId: 'noTryExpect',
node,
});
}
},
CatchClause() {
if (isTest) {
++catchDepth;
}
},
'CatchClause:exit'() {
if (isTest) {
--catchDepth;
}
},
'CallExpression:exit'(node) {
if (isTestCase(node)) {
isTest = false;
}
},
};
},
};