From 9599a09b5cdfbebf9ba2f7d67515b0d6916b5353 Mon Sep 17 00:00:00 2001 From: Mark Skelton Date: Sun, 21 Aug 2022 08:08:26 -0500 Subject: [PATCH] Valid expect (#82) * Add docs and tests * Add valid-expect rule * Docs * Formatting * Support `expect.soft` * Support `expect.poll` * Add rule by default * Docs --- README.md | 26 +++++---- docs/rules/valid-expect.md | 39 +++++++++++++ src/index.ts | 3 + src/rules/valid-expect.ts | 103 +++++++++++++++++++++++++++++++++ src/utils/ast.ts | 11 ++++ src/utils/types.ts | 4 ++ test/spec/valid-expect.spec.ts | 93 +++++++++++++++++++++++++++++ 7 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 docs/rules/valid-expect.md create mode 100644 src/rules/valid-expect.ts create mode 100644 src/utils/types.ts create mode 100644 test/spec/valid-expect.spec.ts diff --git a/README.md b/README.md index 897497b..499f145 100644 --- a/README.md +++ b/README.md @@ -49,15 +49,17 @@ command line option.\ 💡: Some problems reported by this rule are manually fixable by editor [suggestions](https://eslint.org/docs/latest/developer-guide/working-with-rules#providing-suggestions). -| ✔ | 🔧 | 💡 | Rule | Description | -| :-: | :-: | :-: | --------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------- | -| ✔ | | | [max-nested-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/max-nested-describe.md) | Enforces a maximum depth to nested describe calls | -| ✔ | 🔧 | | [missing-playwright-await](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/missing-playwright-await.md) | Enforce Playwright APIs to be awaited | -| ✔ | | | [no-conditional-in-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-in-test.md) | Disallow conditional logic in tests | -| ✔ | | 💡 | [no-element-handle](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-element-handle.md) | Disallow usage of element handles | -| ✔ | | | [no-eval](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-eval.md) | Disallow usage of `page.$eval` and `page.$$eval` | -| ✔ | | 💡 | [no-focused-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-focused-test.md) | Disallow usage of `.only` annotation | -| ✔ | | | [no-force-option](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-force-option.md) | Disallow usage of the `{ force: true }` option | -| ✔ | | | [no-page-pause](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-page-pause.md) | Disallow using `page.pause` | -| ✔ | | 💡 | [no-skipped-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-skipped-test.md) | Disallow usage of the `.skip` annotation | -| ✔ | | 💡 | [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` | +| ✔ | 🔧 | 💡 | Rule | Description | +| :-: | :-: | :-: | --------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | +| ✔ | | | [max-nested-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/max-nested-describe.md) | Enforces a maximum depth to nested describe calls | +| ✔ | 🔧 | | [missing-playwright-await](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/missing-playwright-await.md) | Enforce Playwright APIs to be awaited | +| ✔ | | | [no-conditional-in-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-in-test.md) | Disallow conditional logic in tests | +| ✔ | | 💡 | [no-element-handle](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-element-handle.md) | Disallow usage of element handles | +| ✔ | | | [no-eval](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-eval.md) | Disallow usage of `page.$eval` and `page.$$eval` | +| ✔ | | 💡 | [no-focused-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-focused-test.md) | Disallow usage of `.only` annotation | +| ✔ | | | [no-force-option](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-force-option.md) | Disallow usage of the `{ force: true }` option | +| ✔ | | | [no-page-pause](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-page-pause.md) | Disallow using `page.pause` | +| ✔ | | 💡 | [no-skipped-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-skipped-test.md) | Disallow usage of the `.skip` annotation | +| ✔ | | | [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` | +| ✔ | | | [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/rules/valid-expect.md b/docs/rules/valid-expect.md new file mode 100644 index 0000000..94142d3 --- /dev/null +++ b/docs/rules/valid-expect.md @@ -0,0 +1,39 @@ +# Enforce valid `expect()` usage (`valid-expect`) + +Ensure `expect()` is called with a matcher. + +## Rule details + +Examples of **incorrect** code for this rule: + +```javascript +expect(); +expect('something'); +expect(true).toBeDefined; +``` + +Example of **correct** code for this rule: + +```javascript +expect(locator).toHaveText('howdy'); +expect('something').toBe('something'); +expect(true).toBeDefined(); +``` + +## Options + +```json +{ + "minArgs": 1, + "maxArgs": 2 +} +``` + +### `minArgs` & `maxArgs` + +Enforces the minimum and maximum number of arguments that `expect` can take, and +is required to take. + +`minArgs` defaults to 1 while `maxArgs` deafults to `2` to support custom expect +messages. If you want to enforce `expect` always or never has a custom message, +you can adjust these two option values to your preference. diff --git a/src/index.ts b/src/index.ts index f71e8ad..d18e741 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,6 +9,7 @@ import noForceOption from './rules/no-force-option'; import maxNestedDescribe from './rules/max-nested-describe'; import noConditionalInTest from './rules/no-conditional-in-test'; import noUselessNot from './rules/no-useless-not'; +import validExpect from './rules/valid-expect'; export = { configs: { @@ -30,6 +31,7 @@ export = { 'playwright/max-nested-describe': 'warn', 'playwright/no-conditional-in-test': 'warn', 'playwright/no-useless-not': 'warn', + 'playwright/valid-expect': 'error', }, }, 'jest-playwright': { @@ -77,5 +79,6 @@ export = { 'max-nested-describe': maxNestedDescribe, 'no-conditional-in-test': noConditionalInTest, 'no-useless-not': noUselessNot, + 'valid-expect': validExpect, }, }; diff --git a/src/rules/valid-expect.ts b/src/rules/valid-expect.ts new file mode 100644 index 0000000..c6b6b8b --- /dev/null +++ b/src/rules/valid-expect.ts @@ -0,0 +1,103 @@ +import { Rule } from 'eslint'; +import { isExpectCall, isIdentifier } from '../utils/ast'; +import { NodeWithParent } from '../utils/types'; + +function isMatcherFound(node: NodeWithParent): boolean { + if (node.parent.type !== 'MemberExpression') { + return false; + } + + return !( + isIdentifier(node.parent.property, 'not') && + node.parent.parent.type !== 'MemberExpression' + ); +} + +function isMatcherCalled(node: NodeWithParent): boolean { + return node.parent.type === 'MemberExpression' + ? // If the parent is a member expression, we continue traversing upward to + // handle matcher chains of unknown length. e.g. expect().not.something. + isMatcherCalled(node.parent) + : // Just asserting that the parent is a call expression is not enough as + // the node could be an argument of a call expression which doesn't + // determine if it is called. To determine if it is called, we verify + // that the parent call expression callee is the same as the node. + node.parent.type === 'CallExpression' && node.parent.callee === node; +} + +const getAmountData = (amount: number) => ({ + amount: amount.toString(), + s: amount === 1 ? '' : 's', +}); + +export default { + create(context) { + const options = { + minArgs: 1, + maxArgs: 2, + ...((context.options?.[0] as {}) ?? {}), + }; + + const minArgs = Math.min(options.minArgs, options.maxArgs); + const maxArgs = Math.max(options.minArgs, options.maxArgs); + + return { + CallExpression(node) { + if (!isExpectCall(node)) return; + + if (!isMatcherFound(node)) { + context.report({ node, messageId: 'matcherNotFound' }); + } else if (!isMatcherCalled(node)) { + context.report({ node, messageId: 'matcherNotCalled' }); + } + + if (node.arguments.length < minArgs) { + context.report({ + messageId: 'notEnoughArgs', + data: getAmountData(minArgs), + node, + }); + } + + if (node.arguments.length > maxArgs) { + context.report({ + messageId: 'tooManyArgs', + data: getAmountData(maxArgs), + node, + }); + } + }, + }; + }, + meta: { + docs: { + category: 'Possible Errors', + description: 'Enforce valid `expect()` usage', + recommended: true, + url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-expect.md', + }, + messages: { + tooManyArgs: 'Expect takes at most {{amount}} argument{{s}}.', + notEnoughArgs: 'Expect requires at least {{amount}} argument{{s}}.', + matcherNotFound: 'Expect must have a corresponding matcher call.', + matcherNotCalled: 'Matchers must be called to assert.', + }, + type: 'problem', + schema: [ + { + type: 'object', + properties: { + minArgs: { + type: 'number', + minimum: 1, + }, + maxArgs: { + type: 'number', + minimum: 1, + }, + }, + additionalProperties: false, + }, + ], + }, +} as Rule.RuleModule; diff --git a/src/utils/ast.ts b/src/utils/ast.ts index eb8bdbe..8c8dd23 100644 --- a/src/utils/ast.ts +++ b/src/utils/ast.ts @@ -120,3 +120,14 @@ export function isTest(node: ESTree.CallExpression) { ) ); } + +const expectSubCommands = new Set(['soft', 'poll']); +export function isExpectCall(node: ESTree.CallExpression) { + return ( + isIdentifier(node.callee, 'expect') || + (node.callee.type === 'MemberExpression' && + isIdentifier(node.callee.object, 'expect') && + node.callee.property.type === 'Identifier' && + expectSubCommands.has(node.callee.property.name)) + ); +} diff --git a/src/utils/types.ts b/src/utils/types.ts new file mode 100644 index 0000000..f1bfcd7 --- /dev/null +++ b/src/utils/types.ts @@ -0,0 +1,4 @@ +import { Rule } from 'eslint'; +import * as ESTree from 'estree'; + +export type NodeWithParent = ESTree.Node & Rule.NodeParentExtension; diff --git a/test/spec/valid-expect.spec.ts b/test/spec/valid-expect.spec.ts new file mode 100644 index 0000000..7f47d04 --- /dev/null +++ b/test/spec/valid-expect.spec.ts @@ -0,0 +1,93 @@ +import { runRuleTester } from '../utils/rule-tester'; +import rule from '../../src/rules/valid-expect'; + +const invalid = (code: string, messageId: string) => ({ + code, + errors: [{ messageId }], +}); + +runRuleTester('valid-expect', rule, { + valid: [ + 'expect("something").toBe("else")', + 'expect.soft("something").toBe("else")', + 'expect.poll(() => "something").toBe("else")', + 'expect(true).toBeDefined()', + 'expect(undefined).not.toBeDefined()', + 'expect([1, 2, 3]).toEqual([1, 2, 3])', + 'expect(1, "1 !== 2").toBe(2)', + 'expect.soft(1, "1 !== 2").toBe(2)', + 'expect.poll(() => 1, { message: "1 !== 2" }).toBe(2)', + // minArgs + { + code: 'expect(1, "1 !== 2").toBe(2)', + options: [{ minArgs: 2 }], + }, + { + code: 'expect(1, 2, 3).toBe(4)', + options: [{ minArgs: 3 }], + }, + // maxArgs + { + code: 'expect(1).toBe(2)', + options: [{ maxArgs: 1 }], + }, + { + code: 'expect(1, 2, 3).toBe(4)', + options: [{ maxArgs: 3 }], + }, + ], + invalid: [ + // Matcher not found + invalid('expect(foo)', 'matcherNotFound'), + invalid('expect(foo).not', 'matcherNotFound'), + invalid('expect.soft(foo)', 'matcherNotFound'), + invalid('expect.soft(foo).not', 'matcherNotFound'), + invalid('expect.poll(foo)', 'matcherNotFound'), + invalid('expect.poll(foo).not', 'matcherNotFound'), + // Matcher not called + invalid('expect(foo).toBe', 'matcherNotCalled'), + invalid('expect(foo).not.toBe', 'matcherNotCalled'), + invalid('something(expect(foo).not.toBe)', 'matcherNotCalled'), + invalid('expect.soft(foo).toBe', 'matcherNotCalled'), + invalid('expect.soft(foo).not.toBe', 'matcherNotCalled'), + invalid('something(expect.soft(foo).not.toBe)', 'matcherNotCalled'), + invalid('expect.poll(() => foo).toBe', 'matcherNotCalled'), + invalid('expect.poll(() => foo).not.toBe', 'matcherNotCalled'), + invalid('something(expect.poll(() => foo).not.toBe)', 'matcherNotCalled'), + // minArgs + { + code: 'expect().toBe(true)', + errors: [{ messageId: 'notEnoughArgs', data: { amount: 1, s: '' } }], + }, + { + code: 'expect(foo).toBe(true)', + options: [{ minArgs: 2 }], + errors: [{ messageId: 'notEnoughArgs', data: { amount: 2, s: 's' } }], + }, + // maxArgs + { + code: 'expect(foo, "bar", "baz").toBe(true)', + errors: [{ messageId: 'tooManyArgs', data: { amount: 2, s: 's' } }], + }, + { + code: 'expect(foo, "bar").toBe(true)', + options: [{ maxArgs: 1 }], + errors: [{ messageId: 'tooManyArgs', data: { amount: 1, s: '' } }], + }, + // Multiple errors + { + code: 'expect()', + errors: [ + { messageId: 'matcherNotFound' }, + { messageId: 'notEnoughArgs', data: { amount: 1, s: '' } }, + ], + }, + { + code: 'expect().toHaveText', + errors: [ + { messageId: 'matcherNotCalled' }, + { messageId: 'notEnoughArgs', data: { amount: 1, s: '' } }, + ], + }, + ], +});