diff --git a/README.md b/README.md index bc685b8..cc71c9d 100644 --- a/README.md +++ b/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 | diff --git a/docs/prefer-to-be.md b/docs/prefer-to-be.md new file mode 100644 index 0000000..c8ae750 --- /dev/null +++ b/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 }); +``` diff --git a/src/index.ts b/src/index.ts index 0127471..dbfd7a2 100644 --- a/src/index.ts +++ b/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, diff --git a/src/rules/missing-playwright-await.ts b/src/rules/missing-playwright-await.ts index 0b9285c..3316914 100644 --- a/src/rules/missing-playwright-await.ts +++ b/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 diff --git a/src/rules/prefer-to-be.ts b/src/rules/prefer-to-be.ts new file mode 100644 index 0000000..33d4529 --- /dev/null +++ b/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; diff --git a/src/utils/ast.ts b/src/utils/ast.ts index dd09ed0..5515363 100644 --- a/src/utils/ast.ts +++ b/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; diff --git a/src/utils/fixer.ts b/src/utils/fixer.ts new file mode 100644 index 0000000..a1ae73f --- /dev/null +++ b/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 + ); +} diff --git a/src/utils/parseExpectCall.ts b/src/utils/parseExpectCall.ts new file mode 100644 index 0000000..62fa219 --- /dev/null +++ b/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, + }; +} diff --git a/test/spec/prefer-to-be.spec.ts b/test/spec/prefer-to-be.spec.ts new file mode 100644 index 0000000..4949397 --- /dev/null +++ b/test/spec/prefer-to-be.spec.ts @@ -0,0 +1,298 @@ +import rule from '../../src/rules/prefer-to-be'; +import { runRuleTester } from '../utils/rule-tester'; + +runRuleTester('prefer-to-be', rule, { + valid: [ + 'expect(null).toBeNull();', + 'expect(null).not.toBeNull();', + 'expect(null).toBe(1);', + 'expect(null).toBe(-1);', + 'expect(null).toBe(...1);', + 'expect(obj).toStrictEqual([ x, 1 ]);', + 'expect(obj).toStrictEqual({ x: 1 });', + 'expect(obj).not.toStrictEqual({ x: 1 });', + 'expect(value).toMatchSnapshot();', + "expect(catchError()).toStrictEqual({ message: 'oh noes!' })", + 'expect("something");', + 'expect(token).toStrictEqual(/[abc]+/g);', + "expect(token).toStrictEqual(new RegExp('[abc]+', 'g'));", + 'expect(value).toEqual(dedent`my string`);', + ], + invalid: [ + { + code: 'expect(value).toEqual("my string");', + output: 'expect(value).toBe("my string");', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value).toStrictEqual("my string");', + output: 'expect(value).toBe("my string");', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value).toStrictEqual(1);', + output: 'expect(value).toBe(1);', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value).toStrictEqual(-1);', + output: 'expect(value).toBe(-1);', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value).toEqual(`my string`);', + output: 'expect(value).toBe(`my string`);', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value)["toEqual"](`my string`);', + output: 'expect(value)["toBe"](`my string`);', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(value).toStrictEqual(`my ${string}`);', + output: 'expect(value).toBe(`my ${string}`);', + errors: [{ messageId: 'useToBe', column: 15, line: 1 }], + }, + { + code: 'expect(loadMessage()).resolves.toStrictEqual("hello world");', + output: 'expect(loadMessage()).resolves.toBe("hello world");', + errors: [{ messageId: 'useToBe', column: 32, line: 1 }], + }, + { + code: 'expect(loadMessage()).resolves["toStrictEqual"]("hello world");', + output: 'expect(loadMessage()).resolves["toBe"]("hello world");', + errors: [{ messageId: 'useToBe', column: 32, line: 1 }], + }, + { + code: 'expect(loadMessage())["resolves"].toStrictEqual("hello world");', + output: 'expect(loadMessage())["resolves"].toBe("hello world");', + errors: [{ messageId: 'useToBe', column: 35, line: 1 }], + }, + { + code: 'expect(loadMessage()).resolves.toStrictEqual(false);', + output: 'expect(loadMessage()).resolves.toBe(false);', + errors: [{ messageId: 'useToBe', column: 32, line: 1 }], + }, + ], +}); + +runRuleTester('prefer-to-be: null', rule, { + valid: [ + 'expect(null).toBeNull();', + 'expect(null).not.toBeNull();', + 'expect(null).toBe(1);', + 'expect(obj).toStrictEqual([ x, 1 ]);', + 'expect(obj).toStrictEqual({ x: 1 });', + 'expect(obj).not.toStrictEqual({ x: 1 });', + 'expect(value).toMatchSnapshot();', + "expect(catchError()).toStrictEqual({ message: 'oh noes!' })", + 'expect("something");', + // + 'expect(null).not.toEqual();', + 'expect(null).toBe();', + 'expect(null).toMatchSnapshot();', + 'expect("a string").toMatchSnapshot(null);', + 'expect("a string").not.toMatchSnapshot();', + 'expect(null).toBe', + ], + invalid: [ + { + code: 'expect(null).toBe(null);', + output: 'expect(null).toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 14, line: 1 }], + }, + { + code: 'expect(null).toEqual(null);', + output: 'expect(null).toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 14, line: 1 }], + }, + { + code: 'expect(null).toStrictEqual(null);', + output: 'expect(null).toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 14, line: 1 }], + }, + { + code: 'expect("a string").not.toBe(null);', + output: 'expect("a string").not.toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 24, line: 1 }], + }, + { + code: 'expect("a string").not["toBe"](null);', + output: 'expect("a string").not["toBeNull"]();', + errors: [{ messageId: 'useToBeNull', column: 24, line: 1 }], + }, + { + code: 'expect("a string")["not"]["toBe"](null);', + output: 'expect("a string")["not"]["toBeNull"]();', + errors: [{ messageId: 'useToBeNull', column: 27, line: 1 }], + }, + { + code: 'expect("a string").not.toEqual(null);', + output: 'expect("a string").not.toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 24, line: 1 }], + }, + { + code: 'expect("a string").not.toStrictEqual(null);', + output: 'expect("a string").not.toBeNull();', + errors: [{ messageId: 'useToBeNull', column: 24, line: 1 }], + }, + ], +}); + +runRuleTester('prefer-to-be: undefined', rule, { + valid: [ + 'expect(undefined).toBeUndefined();', + 'expect(true).toBeDefined();', + 'expect({}).toEqual({});', + 'expect(something).toBe()', + 'expect(something).toBe(somethingElse)', + 'expect(something).toEqual(somethingElse)', + 'expect(something).not.toBe(somethingElse)', + 'expect(something).not.toEqual(somethingElse)', + 'expect(undefined).toBe', + 'expect("something");', + ], + + invalid: [ + { + code: 'expect(undefined).toBe(undefined);', + output: 'expect(undefined).toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 19, line: 1 }], + }, + { + code: 'expect(undefined).toEqual(undefined);', + output: 'expect(undefined).toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 19, line: 1 }], + }, + { + code: 'expect(undefined).toStrictEqual(undefined);', + output: 'expect(undefined).toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 19, line: 1 }], + }, + { + code: 'expect("a string").not.toBe(undefined);', + output: 'expect("a string").toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 24, line: 1 }], + }, + { + code: 'expect("a string").rejects.not.toBe(undefined);', + output: 'expect("a string").rejects.toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 32, line: 1 }], + }, + { + code: 'expect("a string").rejects.not["toBe"](undefined);', + output: 'expect("a string").rejects["toBeDefined"]();', + errors: [{ messageId: 'useToBeDefined', column: 32, line: 1 }], + }, + { + code: 'expect("a string").not.toEqual(undefined);', + output: 'expect("a string").toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 24, line: 1 }], + }, + { + code: 'expect("a string").not.toStrictEqual(undefined);', + output: 'expect("a string").toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 24, line: 1 }], + }, + ], +}); + +runRuleTester('prefer-to-be: NaN', rule, { + valid: [ + 'expect(NaN).toBeNaN();', + 'expect(true).not.toBeNaN();', + 'expect({}).toEqual({});', + 'expect(something).toBe()', + 'expect(something).toBe(somethingElse)', + 'expect(something).toEqual(somethingElse)', + 'expect(something).not.toBe(somethingElse)', + 'expect(something).not.toEqual(somethingElse)', + 'expect(undefined).toBe', + 'expect("something");', + ], + invalid: [ + { + code: 'expect(NaN).toBe(NaN);', + output: 'expect(NaN).toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 13, line: 1 }], + }, + { + code: 'expect(NaN).toEqual(NaN);', + output: 'expect(NaN).toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 13, line: 1 }], + }, + { + code: 'expect(NaN).toStrictEqual(NaN);', + output: 'expect(NaN).toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 13, line: 1 }], + }, + { + code: 'expect("a string").not.toBe(NaN);', + output: 'expect("a string").not.toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 24, line: 1 }], + }, + { + code: 'expect("a string").rejects.not.toBe(NaN);', + output: 'expect("a string").rejects.not.toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 32, line: 1 }], + }, + { + code: 'expect("a string")["rejects"].not.toBe(NaN);', + output: 'expect("a string")["rejects"].not.toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 35, line: 1 }], + }, + { + code: 'expect("a string").not.toEqual(NaN);', + output: 'expect("a string").not.toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 24, line: 1 }], + }, + { + code: 'expect("a string").not.toStrictEqual(NaN);', + output: 'expect("a string").not.toBeNaN();', + errors: [{ messageId: 'useToBeNaN', column: 24, line: 1 }], + }, + ], +}); + +runRuleTester('prefer-to-be: undefined vs defined', rule, { + valid: [ + 'expect(NaN).toBeNaN();', + 'expect(true).not.toBeNaN();', + 'expect({}).toEqual({});', + 'expect(something).toBe()', + 'expect(something).toBe(somethingElse)', + 'expect(something).toEqual(somethingElse)', + 'expect(something).not.toBe(somethingElse)', + 'expect(something).not.toEqual(somethingElse)', + 'expect(undefined).toBe', + 'expect("something");', + ], + invalid: [ + { + code: 'expect(undefined).not.toBeDefined();', + output: 'expect(undefined).toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 23, line: 1 }], + }, + { + code: 'expect(undefined).resolves.not.toBeDefined();', + output: 'expect(undefined).resolves.toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 32, line: 1 }], + }, + { + code: 'expect(undefined).resolves.toBe(undefined);', + output: 'expect(undefined).resolves.toBeUndefined();', + errors: [{ messageId: 'useToBeUndefined', column: 28, line: 1 }], + }, + { + code: 'expect("a string").not.toBeUndefined();', + output: 'expect("a string").toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 24, line: 1 }], + }, + { + code: 'expect("a string").rejects.not.toBeUndefined();', + output: 'expect("a string").rejects.toBeDefined();', + errors: [{ messageId: 'useToBeDefined', column: 32, line: 1 }], + }, + ], +});