Add prefer-to-be (#106)

* Add prefer-to-be

* Cleanup

README.md

@@ -64,6 +64,7 @@ command line 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-timeout](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-wait-for-timeout.md)               | Disallow usage of `page.waitForTimeout`                           |
 |     | 🔧  |     | [prefer-lowercase-title](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-lowercase-title.md)         | Enforce lowercase test names                                      |
+|     | 🔧  |     | [prefer-to-be](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-to-be.md)                             | Suggest using `toBe()`                                            |
 |     | 🔧  |     | [prefer-to-have-length](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-to-have-length.md)           | Suggest using `toHaveLength()`                                    |
 |     |     |     | [require-top-level-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `test.describe` block |
 |  ✔  |     |     | [valid-expect](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-expect.md)                             | Enforce valid `expect()` usage                                    |

docs/prefer-to-be.md

@@ -0,0 +1,51 @@
+# Suggest using `toBe()` for primitive literals (`prefer-to-be`)
+
+When asserting against primitive literals such as numbers and strings, the
+equality matchers all operate the same, but read slightly differently in code.
+
+This rule recommends using the `toBe` matcher in these situations, as it forms
+the most grammatically natural sentence. For `null`, `undefined`, and `NaN` this
+rule recommends using their specific `toBe` matchers, as they give better error
+messages as well.
+
+## Rule details
+
+This rule triggers a warning if `toEqual()` or `toStrictEqual()` are used to
+assert a primitive literal value such as numbers, strings, and booleans.
+
+The following patterns are considered warnings:
+
+```javascript
+expect(value).not.toEqual(5);
+expect(getMessage()).toStrictEqual('hello world');
+expect(loadMessage()).resolves.toEqual('hello world');
+```
+
+The following pattern is not warning:
+
+```javascript
+expect(value).not.toBe(5);
+expect(getMessage()).toBe('hello world');
+expect(loadMessage()).resolves.toBe('hello world');
+expect(didError).not.toBe(true);
+expect(catchError()).toStrictEqual({ message: 'oh noes!' });
+```
+
+For `null`, `undefined`, and `NaN`, this rule triggers a warning if `toBe` is
+used to assert against those literal values instead of their more specific
+`toBe` counterparts:
+
+```javascript
+expect(value).not.toBe(undefined);
+expect(getMessage()).toBe(null);
+expect(countMessages()).resolves.not.toBe(NaN);
+```
+
+The following pattern is not warning:
+
+```javascript
+expect(value).toBeDefined();
+expect(getMessage()).toBeNull();
+expect(countMessages()).resolves.not.toBeNaN();
+expect(catchError()).toStrictEqual({ message: undefined });
+```

src/index.ts

@@ -11,6 +11,7 @@ import noConditionalInTest from './rules/no-conditional-in-test';
 import noRestrictedMatchers from './rules/no-restricted-matchers';
 import noUselessNot from './rules/no-useless-not';
 import preferLowercaseTitle from './rules/prefer-lowercase-title';
+import preferToBe from './rules/prefer-to-be';
 import preferToHaveLength from './rules/prefer-to-have-length';
 import requireTopLevelDescribe from './rules/require-top-level-describe';
 import validExpect from './rules/valid-expect';
@@ -85,6 +86,7 @@ export = {
     'no-useless-not': noUselessNot,
     'no-restricted-matchers': noRestrictedMatchers,
     'prefer-lowercase-title': preferLowercaseTitle,
+    'prefer-to-be': preferToBe,
     'prefer-to-have-length': preferToHaveLength,
     'require-top-level-describe': requireTopLevelDescribe,
     'valid-expect': validExpect,

src/rules/missing-playwright-await.ts

@@ -3,7 +3,7 @@ import { Rule } from 'eslint';
 import {
   getMatchers,
   isPropertyAccessor,
-  parseExpectCall,
+  getExpectType,
   isIdentifier,
   getStringValue,
 } from '../utils/ast';
@@ -70,7 +70,7 @@ function getCallType(
     return { messageId: 'testStep' };
   }
 
-  const expectType = parseExpectCall(node);
+  const expectType = getExpectType(node);
   if (!expectType) return;
 
   // expect.poll

src/rules/prefer-to-be.ts

@@ -0,0 +1,116 @@
+import { Rule } from 'eslint';
+import * as ESTree from 'estree';
+import { getStringValue, isIdentifier } from '../utils/ast';
+import { replaceAccessorFixer } from '../utils/fixer';
+import { ParsedExpectCall, parseExpectCall } from '../utils/parseExpectCall';
+
+function shouldUseToBe(expectCall: ParsedExpectCall) {
+  let arg = expectCall.args[0];
+
+  if (arg.type === 'UnaryExpression' && arg.operator === '-') {
+    arg = arg.argument;
+  }
+
+  if (arg.type === 'Literal') {
+    // regex literals are classed as literals, but they're actually objects
+    // which means "toBe" will give different results than other matchers
+    return !('regex' in arg);
+  }
+
+  return arg.type === 'TemplateLiteral';
+}
+
+function reportPreferToBe(
+  context: Rule.RuleContext,
+  expectCall: ParsedExpectCall,
+  whatToBe: string,
+  notModifier?: ESTree.Node
+) {
+  context.report({
+    node: expectCall.matcher,
+    messageId: `useToBe${whatToBe}`,
+    fix(fixer) {
+      const fixes = [
+        replaceAccessorFixer(fixer, expectCall.matcher, `toBe${whatToBe}`),
+      ];
+
+      if (expectCall.args?.length && whatToBe !== '') {
+        fixes.push(fixer.remove(expectCall.args[0]));
+      }
+
+      if (notModifier) {
+        const [start, end] = notModifier.range!;
+        fixes.push(fixer.removeRange([start - 1, end]));
+      }
+
+      return fixes;
+    },
+  });
+}
+
+export default {
+  create(context) {
+    return {
+      CallExpression(node) {
+        const expectCall = parseExpectCall(node);
+        if (!expectCall) return;
+
+        const notMatchers = ['toBeUndefined', 'toBeDefined'];
+        const notModifier = expectCall.modifiers.find(
+          (node) => getStringValue(node) === 'not'
+        );
+
+        if (notModifier && notMatchers.includes(expectCall.matcherName)) {
+          return reportPreferToBe(
+            context,
+            expectCall,
+            expectCall.matcherName === 'toBeDefined' ? 'Undefined' : 'Defined',
+            notModifier
+          );
+        }
+
+        const argumentMatchers = ['toBe', 'toEqual', 'toStrictEqual'];
+        const firstArg = expectCall.args[0];
+
+        if (!argumentMatchers.includes(expectCall.matcherName) || !firstArg) {
+          return;
+        }
+
+        if (firstArg.type === 'Literal' && firstArg.value === null) {
+          return reportPreferToBe(context, expectCall, 'Null');
+        }
+
+        if (isIdentifier(firstArg, 'undefined')) {
+          const name = notModifier ? 'Defined' : 'Undefined';
+          return reportPreferToBe(context, expectCall, name, notModifier);
+        }
+
+        if (isIdentifier(firstArg, 'NaN')) {
+          return reportPreferToBe(context, expectCall, 'NaN');
+        }
+
+        if (shouldUseToBe(expectCall) && expectCall.matcherName !== 'toBe') {
+          reportPreferToBe(context, expectCall, '');
+        }
+      },
+    };
+  },
+  meta: {
+    docs: {
+      category: 'Best Practices',
+      description: 'Suggest using `toBe()` for primitive literals',
+      recommended: false,
+      url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/prefer-to-be.md',
+    },
+    messages: {
+      useToBe: 'Use `toBe` when expecting primitive literals',
+      useToBeUndefined: 'Use `toBeUndefined` instead',
+      useToBeDefined: 'Use `toBeDefined` instead',
+      useToBeNull: 'Use `toBeNull` instead',
+      useToBeNaN: 'Use `toBeNaN` instead',
+    },
+    fixable: 'code',
+    type: 'suggestion',
+    schema: [],
+  },
+} as Rule.RuleModule;

src/utils/ast.ts

@@ -124,7 +124,7 @@ export function isTestHook(node: ESTree.CallExpression) {
 const expectSubCommands = new Set(['soft', 'poll']);
 export type ExpectType = 'standalone' | 'soft' | 'poll';
 
-export function parseExpectCall(
+export function getExpectType(
   node: ESTree.CallExpression
 ): ExpectType | undefined {
   if (isIdentifier(node.callee, 'expect')) {
@@ -141,15 +141,18 @@ export function parseExpectCall(
 }
 
 export function isExpectCall(node: ESTree.CallExpression) {
-  return !!parseExpectCall(node);
+  return !!getExpectType(node);
 }
 
 export function getMatchers(
-  node: ESTree.Node & Rule.NodeParentExtension,
-  chain: ESTree.Node[] = []
-): ESTree.Node[] {
+  node: Rule.Node,
+  chain: Rule.Node[] = []
+): Rule.Node[] {
   if (node.parent.type === 'MemberExpression' && node.parent.object === node) {
-    return getMatchers(node.parent, [...chain, node.parent.property]);
+    return getMatchers(node.parent, [
+      ...chain,
+      node.parent.property as Rule.Node,
+    ]);
   }
 
   return chain;

src/utils/fixer.ts

@@ -0,0 +1,23 @@
+import { Rule } from 'eslint';
+import * as ESTree from 'estree';
+
+const getOffset = (node: ESTree.Node) => (node.type === 'Identifier' ? 0 : 1);
+
+/**
+ * Replaces an accessor node with the given `text`.
+ *
+ * This ensures that fixes produce valid code when replacing both dot-based and
+ * bracket-based property accessors.
+ */
+export function replaceAccessorFixer(
+  fixer: Rule.RuleFixer,
+  node: ESTree.Node,
+  text: string
+) {
+  const [start, end] = node.range!;
+
+  return fixer.replaceTextRange(
+    [start + getOffset(node), end - getOffset(node)],
+    text
+  );
+}

src/utils/parseExpectCall.ts

@@ -0,0 +1,49 @@
+import { Rule } from 'eslint';
+import { isExpectCall, getMatchers, getStringValue } from './ast';
+import * as ESTree from 'estree';
+
+const MODIFIER_NAMES = new Set(['not', 'resolves', 'rejects']);
+
+function getExpectArguments(node: Rule.Node): ESTree.Node[] {
+  const grandparent = node.parent.parent;
+  return grandparent.type === 'CallExpression' ? grandparent.arguments : [];
+}
+
+export interface ParsedExpectCall {
+  matcherName: string;
+  matcher: ESTree.Node;
+  modifiers: ESTree.Node[];
+  args: ESTree.Node[];
+}
+
+export function parseExpectCall(
+  node: ESTree.CallExpression & Rule.NodeParentExtension
+): ParsedExpectCall | undefined {
+  if (!isExpectCall(node)) {
+    return;
+  }
+
+  const modifiers: ESTree.Node[] = [];
+  let matcher: Rule.Node | undefined;
+
+  // Separate the matchers (e.g. toBe) from modifiers (e.g. not)
+  getMatchers(node).forEach((item) => {
+    if (MODIFIER_NAMES.has(getStringValue(item))) {
+      modifiers.push(item);
+    } else {
+      matcher = item;
+    }
+  });
+
+  // Rules only run against full expect calls with matchers
+  if (!matcher) {
+    return;
+  }
+
+  return {
+    matcherName: getStringValue(matcher),
+    matcher,
+    args: getExpectArguments(matcher),
+    modifiers,
+  };
+}