From c0bd18e587f5881ce12c42fef49667eb58f089d7 Mon Sep 17 00:00:00 2001 From: Mark Skelton Date: Mon, 26 Feb 2024 19:39:00 -0600 Subject: [PATCH 1/2] feat: Add `valid-describe-callback` rule --- README.md | 1 + docs/rules/valid-describe-callback.md | 53 ++++++ src/rules/valid-describe-callback.test.ts | 197 ++++++++++++++++++++++ src/rules/valid-describe-callback.ts | 107 ++++++++++++ 4 files changed, 358 insertions(+) create mode 100644 docs/rules/valid-describe-callback.md create mode 100644 src/rules/valid-describe-callback.test.ts create mode 100644 src/rules/valid-describe-callback.ts diff --git a/README.md b/README.md index 72f919a..c0afb51 100644 --- a/README.md +++ b/README.md @@ -160,5 +160,6 @@ CLI option\ | [require-hook](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/require-hook.md) | Require setup and teardown code to be within a hook | | | | | [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 | | | | | [require-soft-assertions](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/require-soft-assertions.md) | Require assertions to use `expect.soft()` | | 🔧 | | +| [valid-describe-callback](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-describe-callback.md) | Enforce valid `describe()` callback | ✅ | | | | [valid-expect](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ✅ | | | | [valid-title](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-title.md) | Enforce valid titles | ✅ | 🔧 | | diff --git a/docs/rules/valid-describe-callback.md b/docs/rules/valid-describe-callback.md new file mode 100644 index 0000000..1c8988d --- /dev/null +++ b/docs/rules/valid-describe-callback.md @@ -0,0 +1,53 @@ +# Enforce valid `describe()` callback (`valid-describe-callback`) + +Using an improper `describe()` callback function can lead to unexpected test +errors. + +## Rule details + +This rule validates that the second parameter of a `describe()` function is a +callback function. This callback function: + +- should not be + [async](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function) +- should not contain any parameters +- should not contain any `return` statements + +The following patterns are considered warnings: + +```js +// Async callback functions are not allowed +test.describe('myFunction()', async () => { + // ... +}); + +// Callback function parameters are not allowed +test.describe('myFunction()', (done) => { + // ... +}); + +// No return statements are allowed in block of a callback function +test.describe('myFunction', () => { + return Promise.resolve().then(() => { + test('breaks', () => { + throw new Error('Fail'); + }); + }); +}); + +// Returning a value from a describe block is not allowed +test.describe('myFunction', () => + test('returns a truthy value', () => { + expect(myFunction()).toBeTruthy(); + })); +``` + +The following patterns are **not** considered warnings: + +```js +test.describe('myFunction()', () => { + test('returns a truthy value', () => { + expect(myFunction()).toBeTruthy(); + }); +}); +``` diff --git a/src/rules/valid-describe-callback.test.ts b/src/rules/valid-describe-callback.test.ts new file mode 100644 index 0000000..be20026 --- /dev/null +++ b/src/rules/valid-describe-callback.test.ts @@ -0,0 +1,197 @@ +import dedent from 'dedent'; +import { runRuleTester } from '../utils/rule-tester'; +import rule from './valid-describe-callback'; + +runRuleTester('valid-describe-callback', rule, { + invalid: [ + { + code: 'test.describe(() => {})', + errors: [{ column: 15, line: 1, messageId: 'nameAndCallback' }], + }, + { + code: 'describe(() => {})', + errors: [{ column: 10, line: 1, messageId: 'nameAndCallback' }], + }, + { + code: 'test.describe("foo")', + errors: [{ column: 15, line: 1, messageId: 'nameAndCallback' }], + }, + { + code: 'test.describe("foo", "foo2")', + errors: [ + { column: 15, line: 1, messageId: 'secondArgumentMustBeFunction' }, + ], + }, + { + code: 'test.describe()', + errors: [{ column: 1, line: 1, messageId: 'nameAndCallback' }], + }, + { + code: 'test.describe("foo", async () => {})', + errors: [{ column: 22, line: 1, messageId: 'noAsyncDescribeCallback' }], + }, + { + code: 'test.describe("foo", async function () {})', + errors: [{ column: 22, line: 1, messageId: 'noAsyncDescribeCallback' }], + }, + { + code: 'test.describe.only("foo", async function () {})', + errors: [{ column: 27, line: 1, messageId: 'noAsyncDescribeCallback' }], + }, + { + code: 'test.describe.skip("foo", async function () {})', + errors: [{ column: 27, line: 1, messageId: 'noAsyncDescribeCallback' }], + }, + { + code: dedent` + test.describe('sample case', () => { + test('works', () => { + expect(true).toEqual(true); + }); + test.describe('async', async () => { + await new Promise(setImmediate); + test('breaks', () => { + throw new Error('Fail'); + }); + }); + }); + `, + errors: [{ column: 26, line: 5, messageId: 'noAsyncDescribeCallback' }], + }, + { + code: dedent` + test.describe('foo', function () { + return Promise.resolve().then(() => { + test('breaks', () => { + throw new Error('Fail') + }) + }) + }) + `, + errors: [{ column: 3, line: 2, messageId: 'unexpectedReturnInDescribe' }], + }, + { + code: dedent` + test.describe('foo', () => { + return Promise.resolve().then(() => { + test('breaks', () => { + throw new Error('Fail') + }) + }) + test.describe('nested', () => { + return Promise.resolve().then(() => { + test('breaks', () => { + throw new Error('Fail') + }) + }) + }) + }) + `, + errors: [ + { column: 3, line: 2, messageId: 'unexpectedReturnInDescribe' }, + { column: 5, line: 8, messageId: 'unexpectedReturnInDescribe' }, + ], + }, + { + code: dedent` + test.describe('foo', async () => { + await something() + test('does something') + test.describe('nested', () => { + return Promise.resolve().then(() => { + test('breaks', () => { + throw new Error('Fail') + }) + }) + }) + }) + `, + errors: [ + { column: 22, line: 1, messageId: 'noAsyncDescribeCallback' }, + { column: 5, line: 5, messageId: 'unexpectedReturnInDescribe' }, + ], + }, + { + code: dedent` + test.describe('foo', () => + test('bar', () => {}) + ) + `, + errors: [ + { column: 22, line: 1, messageId: 'unexpectedReturnInDescribe' }, + ], + }, + { + code: 'describe("foo", done => {})', + errors: [ + { column: 17, line: 1, messageId: 'unexpectedDescribeArgument' }, + ], + }, + { + code: 'describe("foo", function (done) {})', + errors: [ + { column: 27, line: 1, messageId: 'unexpectedDescribeArgument' }, + ], + }, + { + code: 'describe("foo", function (one, two, three) {})', + errors: [ + { column: 27, line: 1, messageId: 'unexpectedDescribeArgument' }, + ], + }, + { + code: 'describe("foo", async function (done) {})', + errors: [ + { column: 17, line: 1, messageId: 'noAsyncDescribeCallback' }, + { column: 33, line: 1, messageId: 'unexpectedDescribeArgument' }, + ], + }, + // Global aliases + { + code: 'it.describe(() => {})', + errors: [{ column: 13, line: 1, messageId: 'nameAndCallback' }], + settings: { + playwright: { + globalAliases: { test: ['it'] }, + }, + }, + }, + ], + valid: [ + 'test.describe("foo", function() {})', + 'test.describe("foo", () => {})', + 'test.describe(`foo`, () => {})', + 'test.describe.only("foo", () => {})', + 'describe("foo", () => {})', + 'describe.only("foo", () => {})', + dedent` + test.describe('foo', () => { + test('bar', () => { + return Promise.resolve(42).then(value => { + expect(value).toBe(42) + }) + }) + }) + `, + dedent` + test.describe('foo', () => { + test('bar', async () => { + expect(await Promise.resolve(42)).toBe(42) + }) + }) + `, + dedent` + if (hasOwnProperty(obj, key)) { + } + `, + // Global aliases + { + code: 'it.describe("foo", function() {})', + settings: { + playwright: { + globalAliases: { test: ['it'] }, + }, + }, + }, + ], +}); diff --git a/src/rules/valid-describe-callback.ts b/src/rules/valid-describe-callback.ts new file mode 100644 index 0000000..515546e --- /dev/null +++ b/src/rules/valid-describe-callback.ts @@ -0,0 +1,107 @@ +import { Rule } from 'eslint'; +import * as ESTree from 'estree'; +import { getStringValue, isFunction } from '../utils/ast'; +import { parseFnCall } from '../utils/parseFnCall'; + +const paramsLocation = ( + params: ESTree.CallExpression['arguments'] | ESTree.Pattern[], +) => { + const [first] = params; + const last = params[params.length - 1]; + + return { + end: last.loc!.end, + start: first.loc!.start, + }; +}; + +export default { + create(context) { + return { + CallExpression(node) { + const call = parseFnCall(context, node); + if (call?.type !== 'describe') return; + + if (node.arguments.length < 1) { + return context.report({ + loc: node.loc!, + messageId: 'nameAndCallback', + }); + } + + const [, callback] = node.arguments; + + if (!callback) { + context.report({ + loc: paramsLocation(node.arguments), + messageId: 'nameAndCallback', + }); + + return; + } + + if (!isFunction(callback)) { + context.report({ + loc: paramsLocation(node.arguments), + messageId: 'secondArgumentMustBeFunction', + }); + + return; + } + + if (callback.async) { + context.report({ + messageId: 'noAsyncDescribeCallback', + node: callback, + }); + } + + if ( + call.members.every((s) => getStringValue(s) !== 'each') && + callback.params.length + ) { + context.report({ + loc: paramsLocation(callback.params), + messageId: 'unexpectedDescribeArgument', + }); + } + + if (callback.body.type === 'CallExpression') { + context.report({ + messageId: 'unexpectedReturnInDescribe', + node: callback, + }); + } + + if (callback.body.type === 'BlockStatement') { + callback.body.body.forEach((node) => { + if (node.type === 'ReturnStatement') { + context.report({ + messageId: 'unexpectedReturnInDescribe', + node, + }); + } + }); + } + }, + }; + }, + meta: { + docs: { + category: 'Possible Errors', + description: 'Enforce valid `describe()` callback', + recommended: true, + url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/valid-describe-callback.md', + }, + messages: { + nameAndCallback: 'Describe requires name and callback arguments', + noAsyncDescribeCallback: 'No async describe callback', + secondArgumentMustBeFunction: 'Second argument must be function', + unexpectedDescribeArgument: 'Unexpected argument(s) in describe callback', + unexpectedReturnInDescribe: + 'Unexpected return statement in describe callback', + }, + schema: [], + type: 'problem', + }, +} as Rule.RuleModule; From c5481f072071c5e59b4d4c1b5dbe050f56dd6603 Mon Sep 17 00:00:00 2001 From: Mark Skelton Date: Mon, 26 Feb 2024 20:27:55 -0600 Subject: [PATCH 2/2] chore: Add rule to index --- src/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/index.ts b/src/index.ts index 1d799c0..f701ec5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -40,6 +40,7 @@ import preferWebFirstAssertions from './rules/prefer-web-first-assertions'; import requireHook from './rules/require-hook'; import requireSoftAssertions from './rules/require-soft-assertions'; import requireTopLevelDescribe from './rules/require-top-level-describe'; +import validDescribeCallback from './rules/valid-describe-callback'; import validExpect from './rules/valid-expect'; import validTitle from './rules/valid-title'; @@ -87,6 +88,7 @@ const index = { 'require-hook': requireHook, 'require-soft-assertions': requireSoftAssertions, 'require-top-level-describe': requireTopLevelDescribe, + 'valid-describe-callback': validDescribeCallback, 'valid-expect': validExpect, 'valid-title': validTitle, }, @@ -115,6 +117,7 @@ const sharedConfig = { 'playwright/no-wait-for-selector': 'warn', 'playwright/no-wait-for-timeout': 'warn', 'playwright/prefer-web-first-assertions': 'error', + 'playwright/valid-describe-callback': 'error', 'playwright/valid-expect': 'error', 'playwright/valid-title': 'error', },