Skip to content

Commit

Permalink
Valid expect (#82)
Browse files Browse the repository at this point in the history
* Add docs and tests

* Add valid-expect rule

* Docs

* Formatting

* Support `expect.soft`

* Support `expect.poll`

* Add rule by default

* Docs
  • Loading branch information
mskelton authored Aug 21, 2022
1 parent 502df06 commit 9599a09
Show file tree
Hide file tree
Showing 7 changed files with 267 additions and 12 deletions.
26 changes: 14 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
39 changes: 39 additions & 0 deletions docs/rules/valid-expect.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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': {
Expand Down Expand Up @@ -77,5 +79,6 @@ export = {
'max-nested-describe': maxNestedDescribe,
'no-conditional-in-test': noConditionalInTest,
'no-useless-not': noUselessNot,
'valid-expect': validExpect,
},
};
103 changes: 103 additions & 0 deletions src/rules/valid-expect.ts
Original file line number Diff line number Diff line change
@@ -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;
11 changes: 11 additions & 0 deletions src/utils/ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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))
);
}
4 changes: 4 additions & 0 deletions src/utils/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { Rule } from 'eslint';
import * as ESTree from 'estree';

export type NodeWithParent = ESTree.Node & Rule.NodeParentExtension;
93 changes: 93 additions & 0 deletions test/spec/valid-expect.spec.ts
Original file line number Diff line number Diff line change
@@ -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: '' } },
],
},
],
});

0 comments on commit 9599a09

Please sign in to comment.