From b34f13322b80c9fb0f51e7cc188046f8e28f69c7 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sun, 11 Apr 2021 12:44:25 +1200 Subject: [PATCH] feat: create `prefer-expect-resolves` rule --- README.md | 85 ++++++++++--------- docs/rules/prefer-expect-resolves.md | 53 ++++++++++++ .../__snapshots__/rules.test.ts.snap | 1 + src/__tests__/rules.test.ts | 2 +- .../__tests__/prefer-expect-resolves.test.ts | 53 ++++++++++++ src/rules/prefer-expect-resolves.ts | 37 ++++++++ 6 files changed, 188 insertions(+), 43 deletions(-) create mode 100644 docs/rules/prefer-expect-resolves.md create mode 100644 src/rules/__tests__/prefer-expect-resolves.test.ts create mode 100644 src/rules/prefer-expect-resolves.ts diff --git a/README.md b/README.md index 2c38ceb35..2fd951f2a 100644 --- a/README.md +++ b/README.md @@ -130,48 +130,49 @@ installations requiring long-term consistency. -| Rule | Description | Configurations | Fixable | -| ---------------------------------------------------------------------------- | --------------------------------------------------------------- | ---------------- | ------------ | -| [consistent-test-it](docs/rules/consistent-test-it.md) | Have control over `test` and `it` usages | | ![fixable][] | -| [expect-expect](docs/rules/expect-expect.md) | Enforce assertion to be made in a test body | ![recommended][] | | -| [lowercase-name](docs/rules/lowercase-name.md) | Enforce lowercase test names | | ![fixable][] | -| [no-alias-methods](docs/rules/no-alias-methods.md) | Disallow alias methods | ![style][] | ![fixable][] | -| [no-commented-out-tests](docs/rules/no-commented-out-tests.md) | Disallow commented out tests | ![recommended][] | | -| [no-conditional-expect](docs/rules/no-conditional-expect.md) | Prevent calling `expect` conditionally | ![recommended][] | | -| [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | ![recommended][] | ![fixable][] | -| [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | ![recommended][] | | -| [no-done-callback](docs/rules/no-done-callback.md) | Avoid using a callback in asynchronous tests and hooks | ![recommended][] | ![suggest][] | -| [no-duplicate-hooks](docs/rules/no-duplicate-hooks.md) | Disallow duplicate setup and teardown hooks | | | -| [no-export](docs/rules/no-export.md) | Disallow using `exports` in files containing tests | ![recommended][] | | -| [no-focused-tests](docs/rules/no-focused-tests.md) | Disallow focused tests | ![recommended][] | ![suggest][] | -| [no-hooks](docs/rules/no-hooks.md) | Disallow setup and teardown hooks | | | -| [no-identical-title](docs/rules/no-identical-title.md) | Disallow identical titles | ![recommended][] | | -| [no-if](docs/rules/no-if.md) | Disallow conditional logic | | | -| [no-interpolation-in-snapshots](docs/rules/no-interpolation-in-snapshots.md) | Disallow string interpolation inside snapshots | ![recommended][] | | -| [no-jasmine-globals](docs/rules/no-jasmine-globals.md) | Disallow Jasmine globals | ![recommended][] | ![fixable][] | -| [no-jest-import](docs/rules/no-jest-import.md) | Disallow importing Jest | ![recommended][] | | -| [no-large-snapshots](docs/rules/no-large-snapshots.md) | disallow large snapshots | | | -| [no-mocks-import](docs/rules/no-mocks-import.md) | Disallow manually importing from `__mocks__` | ![recommended][] | | -| [no-restricted-matchers](docs/rules/no-restricted-matchers.md) | Disallow specific matchers & modifiers | | | -| [no-standalone-expect](docs/rules/no-standalone-expect.md) | Disallow using `expect` outside of `it` or `test` blocks | ![recommended][] | | -| [no-test-prefixes](docs/rules/no-test-prefixes.md) | Use `.only` and `.skip` over `f` and `x` | ![recommended][] | ![fixable][] | -| [no-test-return-statement](docs/rules/no-test-return-statement.md) | Disallow explicitly returning from tests | | | -| [prefer-called-with](docs/rules/prefer-called-with.md) | Suggest using `toBeCalledWith()` or `toHaveBeenCalledWith()` | | | -| [prefer-expect-assertions](docs/rules/prefer-expect-assertions.md) | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | ![suggest][] | -| [prefer-hooks-on-top](docs/rules/prefer-hooks-on-top.md) | Suggest having hooks before any test cases | | | -| [prefer-spy-on](docs/rules/prefer-spy-on.md) | Suggest using `jest.spyOn()` | | ![fixable][] | -| [prefer-strict-equal](docs/rules/prefer-strict-equal.md) | Suggest using `toStrictEqual()` | | ![suggest][] | -| [prefer-to-be-null](docs/rules/prefer-to-be-null.md) | Suggest using `toBeNull()` | ![style][] | ![fixable][] | -| [prefer-to-be-undefined](docs/rules/prefer-to-be-undefined.md) | Suggest using `toBeUndefined()` | ![style][] | ![fixable][] | -| [prefer-to-contain](docs/rules/prefer-to-contain.md) | Suggest using `toContain()` | ![style][] | ![fixable][] | -| [prefer-to-have-length](docs/rules/prefer-to-have-length.md) | Suggest using `toHaveLength()` | ![style][] | ![fixable][] | -| [prefer-todo](docs/rules/prefer-todo.md) | Suggest using `test.todo` | | ![fixable][] | -| [require-to-throw-message](docs/rules/require-to-throw-message.md) | Require a message for `toThrow()` | | | -| [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | | -| [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | ![recommended][] | | -| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended][] | | -| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | ![recommended][] | | -| [valid-title](docs/rules/valid-title.md) | Enforce valid titles | ![recommended][] | ![fixable][] | +| Rule | Description | Configurations | Fixable | +| ---------------------------------------------------------------------------- | ------------------------------------------------------------------- | ---------------- | ------------ | +| [consistent-test-it](docs/rules/consistent-test-it.md) | Have control over `test` and `it` usages | | ![fixable][] | +| [expect-expect](docs/rules/expect-expect.md) | Enforce assertion to be made in a test body | ![recommended][] | | +| [lowercase-name](docs/rules/lowercase-name.md) | Enforce lowercase test names | | ![fixable][] | +| [no-alias-methods](docs/rules/no-alias-methods.md) | Disallow alias methods | ![style][] | ![fixable][] | +| [no-commented-out-tests](docs/rules/no-commented-out-tests.md) | Disallow commented out tests | ![recommended][] | | +| [no-conditional-expect](docs/rules/no-conditional-expect.md) | Prevent calling `expect` conditionally | ![recommended][] | | +| [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | ![recommended][] | ![fixable][] | +| [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | ![recommended][] | | +| [no-done-callback](docs/rules/no-done-callback.md) | Avoid using a callback in asynchronous tests and hooks | ![recommended][] | ![suggest][] | +| [no-duplicate-hooks](docs/rules/no-duplicate-hooks.md) | Disallow duplicate setup and teardown hooks | | | +| [no-export](docs/rules/no-export.md) | Disallow using `exports` in files containing tests | ![recommended][] | | +| [no-focused-tests](docs/rules/no-focused-tests.md) | Disallow focused tests | ![recommended][] | ![suggest][] | +| [no-hooks](docs/rules/no-hooks.md) | Disallow setup and teardown hooks | | | +| [no-identical-title](docs/rules/no-identical-title.md) | Disallow identical titles | ![recommended][] | | +| [no-if](docs/rules/no-if.md) | Disallow conditional logic | | | +| [no-interpolation-in-snapshots](docs/rules/no-interpolation-in-snapshots.md) | Disallow string interpolation inside snapshots | ![recommended][] | | +| [no-jasmine-globals](docs/rules/no-jasmine-globals.md) | Disallow Jasmine globals | ![recommended][] | ![fixable][] | +| [no-jest-import](docs/rules/no-jest-import.md) | Disallow importing Jest | ![recommended][] | | +| [no-large-snapshots](docs/rules/no-large-snapshots.md) | disallow large snapshots | | | +| [no-mocks-import](docs/rules/no-mocks-import.md) | Disallow manually importing from `__mocks__` | ![recommended][] | | +| [no-restricted-matchers](docs/rules/no-restricted-matchers.md) | Disallow specific matchers & modifiers | | | +| [no-standalone-expect](docs/rules/no-standalone-expect.md) | Disallow using `expect` outside of `it` or `test` blocks | ![recommended][] | | +| [no-test-prefixes](docs/rules/no-test-prefixes.md) | Use `.only` and `.skip` over `f` and `x` | ![recommended][] | ![fixable][] | +| [no-test-return-statement](docs/rules/no-test-return-statement.md) | Disallow explicitly returning from tests | | | +| [prefer-called-with](docs/rules/prefer-called-with.md) | Suggest using `toBeCalledWith()` or `toHaveBeenCalledWith()` | | | +| [prefer-expect-assertions](docs/rules/prefer-expect-assertions.md) | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | ![suggest][] | +| [prefer-expect-resolves](docs/rules/prefer-expect-resolves.md) | Prefer `await expect(...).resolves` over `expect(await ...)` syntax | | | +| [prefer-hooks-on-top](docs/rules/prefer-hooks-on-top.md) | Suggest having hooks before any test cases | | | +| [prefer-spy-on](docs/rules/prefer-spy-on.md) | Suggest using `jest.spyOn()` | | ![fixable][] | +| [prefer-strict-equal](docs/rules/prefer-strict-equal.md) | Suggest using `toStrictEqual()` | | ![suggest][] | +| [prefer-to-be-null](docs/rules/prefer-to-be-null.md) | Suggest using `toBeNull()` | ![style][] | ![fixable][] | +| [prefer-to-be-undefined](docs/rules/prefer-to-be-undefined.md) | Suggest using `toBeUndefined()` | ![style][] | ![fixable][] | +| [prefer-to-contain](docs/rules/prefer-to-contain.md) | Suggest using `toContain()` | ![style][] | ![fixable][] | +| [prefer-to-have-length](docs/rules/prefer-to-have-length.md) | Suggest using `toHaveLength()` | ![style][] | ![fixable][] | +| [prefer-todo](docs/rules/prefer-todo.md) | Suggest using `test.todo` | | ![fixable][] | +| [require-to-throw-message](docs/rules/require-to-throw-message.md) | Require a message for `toThrow()` | | | +| [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | | +| [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | ![recommended][] | | +| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended][] | | +| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | ![recommended][] | | +| [valid-title](docs/rules/valid-title.md) | Enforce valid titles | ![recommended][] | ![fixable][] | diff --git a/docs/rules/prefer-expect-resolves.md b/docs/rules/prefer-expect-resolves.md new file mode 100644 index 000000000..baba945b7 --- /dev/null +++ b/docs/rules/prefer-expect-resolves.md @@ -0,0 +1,53 @@ +# Prefer `await expect(...).resolves` over `expect(await ...)` syntax (`prefer-expect-resolves`) + +When working with promises, there are two primary ways you can test the resolved +value: + +1. use the `resolve` modifier on `expect` + (`await expect(...).resolves.` style) +2. `await` the promise and assert against its result + (`expect(await ...).` style) + +While the second style is arguably less dependent on `jest`, if the promise +rejects it will be treated as a general error, resulting in less predictable +behaviour and output from `jest`. + +Additionally, favoring the first style ensures consistency with its `rejects` +counterpart, as there is no way of "awaiting" a rejection. + +## Rule details + +This rule triggers a warning if an `await` is done within an `expect`, and +recommends using `resolves` instead. + +Examples of **incorrect** code for this rule + +```js +it('passes', async () => { + expect(await someValue()).toBe(true); +}); + +it('is true', async () => { + const myPromise = Promise.resolve(true); + + expect(await myPromise).toBe(true); +}); +``` + +Examples of **correct** code for this rule + +```js +it('passes', async () => { + await expect(someValue()).resolves.toBe(true); +}); + +it('is true', async () => { + const myPromise = Promise.resolve(true); + + await expect(myPromise).resolves.toBe(true); +}); + +it('errors', async () => { + await expect(Promise.rejects('oh noes!')).rejects.toThrow('oh noes!'); +}); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index 414f67032..d81cc5bee 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -36,6 +36,7 @@ Object { "jest/no-test-return-statement": "error", "jest/prefer-called-with": "error", "jest/prefer-expect-assertions": "error", + "jest/prefer-expect-resolves": "error", "jest/prefer-hooks-on-top": "error", "jest/prefer-spy-on": "error", "jest/prefer-strict-equal": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index 6a8ff2df1..6703e7a13 100644 --- a/src/__tests__/rules.test.ts +++ b/src/__tests__/rules.test.ts @@ -2,7 +2,7 @@ import { existsSync } from 'fs'; import { resolve } from 'path'; import plugin from '../'; -const numberOfRules = 45; +const numberOfRules = 46; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/prefer-expect-resolves.test.ts b/src/rules/__tests__/prefer-expect-resolves.test.ts new file mode 100644 index 000000000..9b89faaaa --- /dev/null +++ b/src/rules/__tests__/prefer-expect-resolves.test.ts @@ -0,0 +1,53 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import dedent from 'dedent'; +import resolveFrom from 'resolve-from'; +import rule from '../prefer-expect-resolves'; + +const ruleTester = new TSESLint.RuleTester({ + parser: resolveFrom(require.resolve('eslint'), 'espree'), + parserOptions: { + ecmaVersion: 2017, + }, +}); + +ruleTester.run('prefer-expect-resolves', rule, { + valid: [ + dedent` + it('passes', async () => { + await expect(someValue()).resolves.toBe(true); + }); + `, + dedent` + it('is true', async () => { + const myPromise = Promise.resolve(true); + + await expect(myPromise).resolves.toBe(true); + }); + `, + dedent` + it('errors', async () => { + await expect(Promise.rejects('oh noes!')).rejects.toThrow('oh noes!'); + }); + `, + ], + invalid: [ + { + code: dedent` + it('passes', async () => { + expect(await someValue()).toBe(true); + }); + `, + errors: [{ endColumn: 27, column: 10, messageId: 'expectResolves' }], + }, + { + code: dedent` + it('is true', async () => { + const myPromise = Promise.resolve(true); + + expect(await myPromise).toBe(true); + }); + `, + errors: [{ endColumn: 25, column: 10, messageId: 'expectResolves' }], + }, + ], +}); diff --git a/src/rules/prefer-expect-resolves.ts b/src/rules/prefer-expect-resolves.ts new file mode 100644 index 000000000..d4802fb62 --- /dev/null +++ b/src/rules/prefer-expect-resolves.ts @@ -0,0 +1,37 @@ +import { + AST_NODE_TYPES, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { createRule, isExpectCall } from './utils'; + +export default createRule({ + name: __filename, + meta: { + docs: { + category: 'Best Practices', + description: + 'Prefer `await expect(...).resolves` over `expect(await ...)` syntax', + recommended: false, + }, + messages: { + expectResolves: 'Use `await expect(...).resolves instead.', + }, + schema: [], + type: 'suggestion', + }, + defaultOptions: [], + create: context => ({ + CallExpression(node: TSESTree.CallExpression) { + if ( + isExpectCall(node) && + node.arguments.length && + node.arguments[0].type === AST_NODE_TYPES.AwaitExpression + ) { + context.report({ + node: node.arguments[0], + messageId: 'expectResolves', + }); + } + }, + }), +});