diff --git a/README.md b/README.md index fdcde1481..580632896 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,7 @@ installations requiring long-term consistency. | [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-comparison-matcher](docs/rules/prefer-comparison-matcher.md) | Suggest using the built-in comparison matchers | | ![fixable][] | | [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 | | ![fixable][] | | [prefer-hooks-on-top](docs/rules/prefer-hooks-on-top.md) | Suggest having hooks before any test cases | | | diff --git a/docs/rules/prefer-comparison-matcher.md b/docs/rules/prefer-comparison-matcher.md new file mode 100644 index 000000000..44ec8b9f0 --- /dev/null +++ b/docs/rules/prefer-comparison-matcher.md @@ -0,0 +1,55 @@ +# Suggest using the built-in comparison matchers (`prefer-comparison-matcher`) + +Jest has a number of built-in matchers for comparing numbers which allow for +more readable tests and error messages if an expectation fails. + +## Rule details + +This rule checks for comparisons in tests that could be replaced with one of the +following built-in comparison matchers: + +- `toBeGreaterThan` +- `toBeGreaterThanOrEqual` +- `toBeLessThan` +- `toBeLessThanOrEqual` + +Examples of **incorrect** code for this rule: + +```js +expect(x > 5).toBe(true); +expect(x < 7).not.toEqual(true); +expect(x <= y).toStrictEqual(true); +``` + +Examples of **correct** code for this rule: + +```js +expect(x).toBeGreaterThan(5); +expect(x).not.toBeLessThanOrEqual(7); +expect(x).toBeLessThanOrEqual(y); + +// special case - see below +expect(x < 'Carl').toBe(true); +``` + +Note that these matchers only work with numbers and bigints, and that the rule +assumes that any variables on either side of the comparison operator are of one +of those types - this means if you're using the comparison operator with +strings, the fix applied by this rule will result in an error. + +```js +expect(myName).toBeGreaterThanOrEqual(theirName); // Matcher error: received value must be a number or bigint +``` + +The reason for this is that comparing strings with these operators is expected +to be very rare and would mean not being able to have an automatic fixer for +this rule. + +If for some reason you are using these operators to compare strings, you can +disable this rule using an inline +[configuration comment](https://eslint.org/docs/user-guide/configuring/rules#disabling-rules): + +```js +// eslint-disable-next-line jest/prefer-comparison-matcher +expect(myName > theirName).toBe(true); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index ce45d2b14..f4949b970 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -35,6 +35,7 @@ Object { "jest/no-test-prefixes": "error", "jest/no-test-return-statement": "error", "jest/prefer-called-with": "error", + "jest/prefer-comparison-matcher": "error", "jest/prefer-expect-assertions": "error", "jest/prefer-expect-resolves": "error", "jest/prefer-hooks-on-top": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index dc6ac6b4e..d37e1bd4a 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 = 43; +const numberOfRules = 44; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/prefer-comparison-matcher.test.ts b/src/rules/__tests__/prefer-comparison-matcher.test.ts new file mode 100644 index 000000000..8b03e14ba --- /dev/null +++ b/src/rules/__tests__/prefer-comparison-matcher.test.ts @@ -0,0 +1,192 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import rule from '../prefer-comparison-matcher'; +import { espreeParser } from './test-utils'; + +const ruleTester = new TSESLint.RuleTester({ + parser: espreeParser, + parserOptions: { + ecmaVersion: 2015, + }, +}); + +const generateInvalidCases = ( + operator: string, + equalityMatcher: string, + preferredMatcher: string, + preferredMatcherWhenNegated: string, +): Array> => { + return [ + { + code: `expect(value ${operator} 1).${equalityMatcher}(true);`, + output: `expect(value).${preferredMatcher}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1)['${equalityMatcher}'](true);`, + output: `expect(value).${preferredMatcher}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1).${equalityMatcher}(false);`, + output: `expect(value).${preferredMatcherWhenNegated}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher: preferredMatcherWhenNegated }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1)['${equalityMatcher}'](false);`, + output: `expect(value).${preferredMatcherWhenNegated}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher: preferredMatcherWhenNegated }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1).not.${equalityMatcher}(true);`, + output: `expect(value).${preferredMatcherWhenNegated}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher: preferredMatcherWhenNegated }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1)['not'].${equalityMatcher}(true);`, + output: `expect(value).${preferredMatcherWhenNegated}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher: preferredMatcherWhenNegated }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + { + code: `expect(value ${operator} 1).not.${equalityMatcher}(false);`, + output: `expect(value).${preferredMatcher}(1);`, + errors: [ + { + messageId: 'useToBeComparison', + data: { preferredMatcher }, + column: 18 + operator.length, + line: 1, + }, + ], + }, + ]; +}; + +const generateValidStringLiteralCases = (operator: string, matcher: string) => { + return [ + ['x', "'y'"], + ['x', '`y`'], + ['x', '`y${z}`'], + ].reduce((cases, [a, b]) => [ + ...cases, + ...[ + `expect(${a} ${operator} ${b}).${matcher}(true)`, + `expect(${a} ${operator} ${b}).${matcher}(false)`, + `expect(${a} ${operator} ${b}).not.${matcher}(true)`, + `expect(${a} ${operator} ${b}).not.${matcher}(false)`, + `expect(${b} ${operator} ${a}).${matcher}(true)`, + `expect(${b} ${operator} ${a}).${matcher}(false)`, + `expect(${b} ${operator} ${a}).not.${matcher}(true)`, + `expect(${b} ${operator} ${a}).not.${matcher}(false)`, + `expect(${a} ${operator} ${b}).${matcher}(true)`, + `expect(${a} ${operator} ${b}).${matcher}(false)`, + `expect(${a} ${operator} ${b}).not.${matcher}(true)`, + `expect(${a} ${operator} ${b}).not.${matcher}(false)`, + `expect(${b} ${operator} ${a}).${matcher}(true)`, + `expect(${b} ${operator} ${a}).${matcher}(false)`, + `expect(${b} ${operator} ${a}).not.${matcher}(true)`, + `expect(${b} ${operator} ${a}).not.${matcher}(false)`, + `expect(${b} ${operator} ${b}).not.${matcher}(false)`, + ], + ]); +}; + +const testComparisonOperator = ( + operator: string, + preferredMatcher: string, + preferredMatcherWhenNegated: string, +) => { + ruleTester.run(`prefer-to-be-comparison: ${operator}`, rule, { + valid: [ + 'expect()', + 'expect({}).toStrictEqual({})', + `expect(value).${preferredMatcher}(1);`, + `expect(value).${preferredMatcherWhenNegated}(1);`, + `expect(value).not.${preferredMatcher}(1);`, + `expect(value).not.${preferredMatcherWhenNegated}(1);`, + `expect(value).${preferredMatcher}(1);`, + ...['toBe', 'toEqual', 'toStrictEqual'].reduce( + (cases, equalityMatcher) => [ + ...cases, + ...generateValidStringLiteralCases(operator, equalityMatcher), + ], + [], + ), + ], + invalid: ['toBe', 'toEqual', 'toStrictEqual'].reduce< + Array> + >( + (cases, equalityMatcher) => [ + ...cases, + ...generateInvalidCases( + operator, + equalityMatcher, + preferredMatcher, + preferredMatcherWhenNegated, + ), + ], + [], + ), + }); +}; + +testComparisonOperator('>', 'toBeGreaterThan', 'toBeLessThanOrEqual'); +testComparisonOperator('<', 'toBeLessThan', 'toBeGreaterThanOrEqual'); +testComparisonOperator('>=', 'toBeGreaterThanOrEqual', 'toBeLessThan'); +testComparisonOperator('<=', 'toBeLessThanOrEqual', 'toBeGreaterThan'); + +ruleTester.run(`prefer-to-be-comparison`, rule, { + valid: [ + 'expect()', + 'expect({}).toStrictEqual({})', + 'expect(a === b).toBe(true)', + 'expect(a !== 2).toStrictEqual(true)', + 'expect(a === b).not.toEqual(true)', + 'expect(a !== "string").toStrictEqual(true)', + 'expect(5 != a).toBe(true)', + 'expect(a == "string").toBe(true)', + 'expect(a == "string").not.toBe(true)', + ], + invalid: [], +}); diff --git a/src/rules/prefer-comparison-matcher.ts b/src/rules/prefer-comparison-matcher.ts new file mode 100644 index 000000000..caf4fc555 --- /dev/null +++ b/src/rules/prefer-comparison-matcher.ts @@ -0,0 +1,166 @@ +import { + AST_NODE_TYPES, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { + MaybeTypeCast, + ParsedEqualityMatcherCall, + ParsedExpectMatcher, + createRule, + followTypeAssertionChain, + isExpectCall, + isParsedEqualityMatcherCall, + isStringNode, + parseExpectCall, +} from './utils'; + +const isBooleanLiteral = ( + node: TSESTree.Node, +): node is TSESTree.BooleanLiteral => + node.type === AST_NODE_TYPES.Literal && typeof node.value === 'boolean'; + +type ParsedBooleanEqualityMatcherCall = ParsedEqualityMatcherCall< + MaybeTypeCast +>; + +/** + * Checks if the given `ParsedExpectMatcher` is a call to one of the equality matchers, + * with a boolean literal as the sole argument. + * + * @example javascript + * toBe(true); + * toEqual(false); + * + * @param {ParsedExpectMatcher} matcher + * + * @return {matcher is ParsedBooleanEqualityMatcher} + */ +const isBooleanEqualityMatcher = ( + matcher: ParsedExpectMatcher, +): matcher is ParsedBooleanEqualityMatcherCall => + isParsedEqualityMatcherCall(matcher) && + isBooleanLiteral(followTypeAssertionChain(matcher.arguments[0])); + +const isString = (node: TSESTree.Node) => { + return isStringNode(node) || node.type === AST_NODE_TYPES.TemplateLiteral; +}; + +const isComparingToString = (expression: TSESTree.BinaryExpression) => { + return isString(expression.left) || isString(expression.right); +}; + +const invertOperator = (operator: string) => { + switch (operator) { + case '>': + return '<='; + case '<': + return '>='; + case '>=': + return '<'; + case '<=': + return '>'; + } + + return null; +}; + +const determineMatcher = ( + operator: string, + negated: boolean, +): string | null => { + const op = negated ? invertOperator(operator) : operator; + + switch (op) { + case '>': + return 'toBeGreaterThan'; + case '<': + return 'toBeLessThan'; + case '>=': + return 'toBeGreaterThanOrEqual'; + case '<=': + return 'toBeLessThanOrEqual'; + } + + return null; +}; + +export default createRule({ + name: __filename, + meta: { + docs: { + category: 'Best Practices', + description: 'Suggest using the built-in comparison matchers', + recommended: false, + }, + messages: { + useToBeComparison: 'Prefer using `{{ preferredMatcher }}` instead', + }, + fixable: 'code', + type: 'suggestion', + schema: [], + }, + defaultOptions: [], + create(context) { + return { + CallExpression(node) { + if (!isExpectCall(node)) { + return; + } + + const { + expect: { + arguments: [comparison], + range: [, expectCallEnd], + }, + matcher, + modifier, + } = parseExpectCall(node); + + if ( + !matcher || + comparison?.type !== AST_NODE_TYPES.BinaryExpression || + isComparingToString(comparison) || + !isBooleanEqualityMatcher(matcher) + ) { + return; + } + + const preferredMatcher = determineMatcher( + comparison.operator, + followTypeAssertionChain(matcher.arguments[0]).value === !!modifier, + ); + + if (!preferredMatcher) { + return; + } + + context.report({ + fix(fixer) { + const sourceCode = context.getSourceCode(); + + return [ + // replace the comparison argument with the left-hand side of the comparison + fixer.replaceText( + comparison, + sourceCode.getText(comparison.left), + ), + // replace the current matcher & modifier with the preferred matcher + fixer.replaceTextRange( + [expectCallEnd, matcher.node.range[1]], + `.${preferredMatcher}`, + ), + // replace the matcher argument with the right-hand side of the comparison + fixer.replaceText( + matcher.arguments[0], + sourceCode.getText(comparison.right), + ), + ]; + }, + messageId: 'useToBeComparison', + data: { preferredMatcher }, + node: (modifier || matcher).node.property, + }); + }, + }; + }, +});