Skip to content

Commit

Permalink
feat: Add prefer-comparision-matcher rule
Browse files Browse the repository at this point in the history
Fixes #230
  • Loading branch information
mskelton committed Feb 18, 2024
1 parent 7faa278 commit 51fca42
Show file tree
Hide file tree
Showing 5 changed files with 496 additions and 0 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ CLI option\
| [no-useless-not](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-useless-not.md) | Disallow usage of `not` matchers when a specific matcher exists || 🔧 | |
| [no-wait-for-selector](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-wait-for-selector.md) | Disallow usage of `page.waitForSelector()` || | 💡 |
| [no-wait-for-timeout](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-wait-for-timeout.md) | Disallow usage of `page.waitForTimeout()` || | 💡 |
| [prefer-comparison-matcher](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-comparison-matcher.md) | Suggest using the built-in comparison matchers | | 🔧 | |
| [prefer-hooks-in-order](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-hooks-in-order.md) | Prefer having hooks in a consistent order | | | |
| [prefer-hooks-on-top](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-hooks-on-top.md) | Suggest having hooks before any test cases | | | |
| [prefer-strict-equal](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-strict-equal.md) | Suggest using `toStrictEqual()` | | | 💡 |
Expand Down
55 changes: 55 additions & 0 deletions docs/rules/prefer-comparison-matcher.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Suggest using the built-in comparison matchers (`prefer-comparison-matcher`)

Playwright 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 playwright/prefer-comparison-matcher
expect(myName > theirName).toBe(true);
```
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import noUselessAwait from './rules/no-useless-await';
import noUselessNot from './rules/no-useless-not';
import noWaitForSelector from './rules/no-wait-for-selector';
import noWaitForTimeout from './rules/no-wait-for-timeout';
import preferComparisonMatcher from './rules/prefer-comparison-matcher';
import preferHooksInOrder from './rules/prefer-hooks-in-order';
import preferHooksOnTop from './rules/prefer-hooks-on-top';
import preferLowercaseTitle from './rules/prefer-lowercase-title';
Expand Down Expand Up @@ -68,6 +69,7 @@ const index = {
'no-useless-not': noUselessNot,
'no-wait-for-selector': noWaitForSelector,
'no-wait-for-timeout': noWaitForTimeout,
'prefer-comparison-matcher': preferComparisonMatcher,
'prefer-hooks-in-order': preferHooksInOrder,
'prefer-hooks-on-top': preferHooksOnTop,
'prefer-lowercase-title': preferLowercaseTitle,
Expand Down
124 changes: 124 additions & 0 deletions src/rules/prefer-comparison-matcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
import { Rule } from 'eslint';
import * as ESTree from 'estree';
import {
getParent,
getRawValue,
getStringValue,
isBooleanLiteral,
isStringLiteral,
} from '../utils/ast';
import { parseExpectCall } from '../utils/parseExpectCall';

const equalityMatchers = new Set(['toBe', 'toEqual', 'toStrictEqual']);

const isString = (node: ESTree.Node) => {
return isStringLiteral(node) || node.type === 'TemplateLiteral';
};

const isComparingToString = (expression: ESTree.BinaryExpression) => {
return isString(expression.left) || isString(expression.right);
};

const invertedOperators: Record<string, string | undefined> = {
'<': '>=',
'<=': '>',
'>': '<=',
'>=': '<',
};

const operatorMatcher: Record<string, string | undefined> = {
'<': 'toBeLessThan',
'<=': 'toBeLessThanOrEqual',
'>': 'toBeGreaterThan',
'>=': 'toBeGreaterThanOrEqual',
};

const determineMatcher = (
operator: string,
negated: boolean,
): string | null => {
const op = negated ? invertedOperators[operator] : operator;
return operatorMatcher[op!] ?? null;
};

export default {
create(context) {
return {
CallExpression(node) {
const expectCall = parseExpectCall(context, node);
if (!expectCall || expectCall.args.length === 0) return;

const { args, matcher } = expectCall;
const [comparison] = node.arguments;
const expectCallEnd = node.range![1];
const [matcherArg] = args;

if (
comparison?.type !== 'BinaryExpression' ||
isComparingToString(comparison) ||
!equalityMatchers.has(getStringValue(matcher)) ||
!isBooleanLiteral(matcherArg)
) {
return;
}

const hasNot = expectCall.modifiers.some(
(node) => getStringValue(node) === 'not',
);

const preferredMatcher = determineMatcher(
comparison.operator,
getRawValue(matcherArg) === hasNot.toString(),
);

if (!preferredMatcher) {
return;
}

context.report({
data: { preferredMatcher },
fix(fixer) {
// Preserve the existing modifier if it's not a negation
const [modifier] = expectCall.modifiers;
const modifierText =
modifier && getStringValue(modifier) !== 'not'
? `.${getStringValue(modifier)}`
: '';

return [
// Replace the comparison argument with the left-hand side of the comparison
fixer.replaceText(
comparison,
context.sourceCode.getText(comparison.left),
),
// Replace the current matcher & modifier with the preferred matcher
fixer.replaceTextRange(
[expectCallEnd, getParent(matcher)!.range![1]],
`${modifierText}.${preferredMatcher}`,
),
// Replace the matcher argument with the right-hand side of the comparison
fixer.replaceText(
matcherArg,
context.sourceCode.getText(comparison.right),
),
];
},
messageId: 'useToBeComparison',
node: matcher,
});
},
};
},
meta: {
docs: {
category: 'Best Practices',
description: 'Suggest using the built-in comparison matchers',
recommended: false,
},
fixable: 'code',
messages: {
useToBeComparison: 'Prefer using `{{ preferredMatcher }}` instead',
},
type: 'suggestion',
},
} as Rule.RuleModule;
Loading

0 comments on commit 51fca42

Please sign in to comment.