From ae8f6ad141293541f20320dbebf28e216dbdaa80 Mon Sep 17 00:00:00 2001 From: cartogram Date: Sun, 21 Jul 2019 04:53:04 -0400 Subject: [PATCH 1/4] feat(rules): no-try-expect --- README.md | 2 + docs/rules/no-try-expect.md | 42 +++++++++++++++ src/__tests__/rules.test.js | 2 +- src/rules/__tests__/no-try-expect.test.js | 66 +++++++++++++++++++++++ src/rules/no-try-expect.js | 53 ++++++++++++++++++ 5 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 docs/rules/no-try-expect.md create mode 100644 src/rules/__tests__/no-try-expect.test.js create mode 100644 src/rules/no-try-expect.js diff --git a/README.md b/README.md index 2bffe40c9..58397e876 100644 --- a/README.md +++ b/README.md @@ -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][] | @@ -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 diff --git a/docs/rules/no-try-expect.md b/docs/rules/no-try-expect.md new file mode 100644 index 000000000..765ac9b54 --- /dev/null +++ b/docs/rules/no-try-expect.md @@ -0,0 +1,42 @@ +# 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, Shopify rarely uses this feature. Using +`toThrow` concisely guarantees that an exception was thrown, and that its +contents match expectations. + +The following patterns are warnings: + +```js +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: + +```js +it('foo', () => { + expect(() => foo()).toThrow(/foo error/); +}); + +it('bar', async () => { + await expect(fooPromise).rejects.toThrow(/foo error/); +}); +``` diff --git a/src/__tests__/rules.test.js b/src/__tests__/rules.test.js index b259c2c38..aa2882b6f 100644 --- a/src/__tests__/rules.test.js +++ b/src/__tests__/rules.test.js @@ -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', () => { diff --git a/src/rules/__tests__/no-try-expect.test.js b/src/rules/__tests__/no-try-expect.test.js new file mode 100644 index 000000000..671a39d2d --- /dev/null +++ b/src/rules/__tests__/no-try-expect.test.js @@ -0,0 +1,66 @@ +import { RuleTester } from 'eslint'; +import rule from '../no-try-expect'; + +const ruleTester = new RuleTester({ + parser: require.resolve('babel-eslint'), +}); + +const errors = [ + { + messageId: 'noTryExpect', + }, +]; + +ruleTester.run('no-try-catch', rule, { + valid: [ + { + code: `it('foo', () => { + expect('foo').toEqual('foo'); + })`, + }, + { + code: ` + it('foo', () => { + expect('bar').toEqual('bar'); + }); + try { + + } catch { + expect('foo').toEqual('foo'); + }`, + }, + { + code: ` + it.skip('foo'); + try { + + } catch { + expect('foo').toEqual('foo'); + }`, + }, + ], + invalid: [ + { + code: `it('foo', () => { + try { + + } catch (err) { + expect(err).toMatch('Error'); + } + })`, + errors, + }, + { + code: `it('foo', async () => { + await wrapper('production', async () => { + try { + + } catch (err) { + expect(err).toMatch('Error'); + } + }) + })`, + errors, + }, + ], +}); diff --git a/src/rules/no-try-expect.js b/src/rules/no-try-expect.js new file mode 100644 index 000000000..aa3a424fa --- /dev/null +++ b/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; + } + }, + }; + }, +}; From 32408ad1bb3cec36b173834d7e23840e600b1132 Mon Sep 17 00:00:00 2001 From: cartogram Date: Sun, 21 Jul 2019 08:41:03 -0400 Subject: [PATCH 2/4] docs(no-try-expect): examples and remove Shopify reference --- docs/rules/no-try-expect.md | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/rules/no-try-expect.md b/docs/rules/no-try-expect.md index 765ac9b54..ed2828ecf 100644 --- a/docs/rules/no-try-expect.md +++ b/docs/rules/no-try-expect.md @@ -5,9 +5,9 @@ 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, Shopify rarely uses this feature. Using -`toThrow` concisely guarantees that an exception was thrown, and that its -contents match expectations. +an `expect.assertions(number)` helper, it might be cumbersome to add this to +every single test. Using `toThrow` concisely guarantees that an exception was +thrown, and that its contents match expectations. The following patterns are warnings: @@ -27,6 +27,14 @@ it('bar', async () => { 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: @@ -39,4 +47,10 @@ it('foo', () => { it('bar', async () => { await expect(fooPromise).rejects.toThrow(/foo error/); }); + +it('baz', async () => { + expect(() => foo()).toThrow( + expect.objectContaining({ code: 'MODULE_NOT_FOUND' }), + ); +}); ``` From d9c0935ab5ebd8b867eb041edddd2a07be063acc Mon Sep 17 00:00:00 2001 From: cartogram Date: Sun, 21 Jul 2019 08:41:37 -0400 Subject: [PATCH 3/4] refactor(no-try-expect): test parser and unnecessary braces --- src/rules/__tests__/no-try-expect.test.js | 58 +++++++++++------------ 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/rules/__tests__/no-try-expect.test.js b/src/rules/__tests__/no-try-expect.test.js index 671a39d2d..f8409ed23 100644 --- a/src/rules/__tests__/no-try-expect.test.js +++ b/src/rules/__tests__/no-try-expect.test.js @@ -2,42 +2,30 @@ import { RuleTester } from 'eslint'; import rule from '../no-try-expect'; const ruleTester = new RuleTester({ - parser: require.resolve('babel-eslint'), -}); - -const errors = [ - { - messageId: 'noTryExpect', + parserOptions: { + ecmaVersion: 2019, }, -]; +}); ruleTester.run('no-try-catch', rule, { valid: [ - { - code: `it('foo', () => { + `it('foo', () => { expect('foo').toEqual('foo'); - })`, - }, - { - code: ` - it('foo', () => { - expect('bar').toEqual('bar'); - }); - try { + })`, + `it('foo', () => { + expect('bar').toEqual('bar'); + }); + try { - } catch { - expect('foo').toEqual('foo'); - }`, - }, - { - code: ` - it.skip('foo'); - try { + } catch { + expect('foo').toEqual('foo'); + }`, + `it.skip('foo'); + try { - } catch { - expect('foo').toEqual('foo'); - }`, - }, + } catch { + expect('foo').toEqual('foo'); + }`, ], invalid: [ { @@ -48,7 +36,11 @@ ruleTester.run('no-try-catch', rule, { expect(err).toMatch('Error'); } })`, - errors, + errors: [ + { + messageId: 'noTryExpect', + }, + ], }, { code: `it('foo', async () => { @@ -60,7 +52,11 @@ ruleTester.run('no-try-catch', rule, { } }) })`, - errors, + errors: [ + { + messageId: 'noTryExpect', + }, + ], }, ], }); From 8ac288b685fcdb93421fcf2c057652a398b3a312 Mon Sep 17 00:00:00 2001 From: Matt Seccafien Date: Sun, 21 Jul 2019 09:00:06 -0400 Subject: [PATCH 4/4] Update docs/rules/no-try-expect.md Co-Authored-By: Simen Bekkhus --- docs/rules/no-try-expect.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-try-expect.md b/docs/rules/no-try-expect.md index ed2828ecf..d257bf0bb 100644 --- a/docs/rules/no-try-expect.md +++ b/docs/rules/no-try-expect.md @@ -49,7 +49,7 @@ it('bar', async () => { }); it('baz', async () => { - expect(() => foo()).toThrow( + await expect(() => foo()).rejects.toThrow( expect.objectContaining({ code: 'MODULE_NOT_FOUND' }), ); });