From 621850e459791c9b5509d4f0dee8ef727c81d258 Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Tue, 7 Jun 2022 13:56:31 -0700 Subject: [PATCH 1/5] Add `max-nested-describe` rule and tests --- lib/rules/max-nested-describe.js | 69 ++++++++++ lib/utils/ast.js | 87 ++++++++++++- test/max-nested-describe.spec.js | 208 +++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 2 deletions(-) create mode 100644 lib/rules/max-nested-describe.js create mode 100644 test/max-nested-describe.spec.js diff --git a/lib/rules/max-nested-describe.js b/lib/rules/max-nested-describe.js new file mode 100644 index 0000000..87318ff --- /dev/null +++ b/lib/rules/max-nested-describe.js @@ -0,0 +1,69 @@ + +const { isCallExpression, isDescribeCall } = require('../utils/ast'); + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + create(context) { + const { options } = context; + const defaultOptions = { max: 5 }; + const { max } = options[0] || defaultOptions; + const describeCallbackStack = []; + + function pushDescribeCallback(node) { + const { parent } = node; + + if(!isCallExpression(parent) || !isDescribeCall(parent)) { + return; + } + + describeCallbackStack.push(0); + + if (describeCallbackStack.length > max) { + context.report({ + node: parent, + messageId: 'exceededMaxDepth', + data: { depth: describeCallbackStack.length, max }, + }); + } + } + + function popDescribeCallback(node) { + const { parent } = node; + + if (isCallExpression(parent) && isDescribeCall(parent)) { + describeCallbackStack.pop(); + } + } + + return { + FunctionExpression: pushDescribeCallback, + 'FunctionExpression:exit': popDescribeCallback, + ArrowFunctionExpression: pushDescribeCallback, + 'ArrowFunctionExpression:exit': popDescribeCallback, + }; + }, + meta: { + docs: { + category: 'Best Practices', + description: 'Enforces a maximum depth to nested describe calls', + recommended: false, + }, + messages: { + exceededMaxDepth: 'Too many nested describe calls ({{ depth }}). Maximum allowed is {{ max }}.', + }, + type: 'suggestion', + fixable: 'code', + schema: [ + { + type: 'object', + properties: { + max: { + type: 'integer', + minimum: 0, + }, + }, + additionalProperties: false, + }, + ], + }, +}; diff --git a/lib/utils/ast.js b/lib/utils/ast.js index bd9e630..8ab50a5 100644 --- a/lib/utils/ast.js +++ b/lib/utils/ast.js @@ -37,8 +37,10 @@ function isObjectProperty({ object }, name) { ); } -function isStringLiteral(node) { - return node && node.type === 'Literal' && typeof node.value === 'string'; +function isStringLiteral(node, value) { + return node && node.type === 'Literal' + && typeof node.value === 'string' + && (value === undefined || node.value === value); } function isBooleanLiteral(node) { @@ -49,6 +51,84 @@ function isBinaryExpression(node) { return node && node.type === 'BinaryExpression'; } +function isCallExpression(node) { + return node && node.type === 'CallExpression'; +} + +function isMemberExpression(node) { + return node && node.type === 'MemberExpression'; +} + +function isIdentifier(node, name) { + return node + && node.type === 'Identifier' + && (name === undefined || node.name === name); +} + +function isDescribeAlias(node) { + return isIdentifier(node) && node.name === 'describe'; +} + +function isAccessor(node, value) { + return isIdentifier(node, value) || isStringNode(node, value); +} + +function isStringNode(node, specifics) { + return isStringLiteral(node, specifics) || isTemplateLiteral(node, specifics); +} + +function isTemplateLiteral(node, value) { + return node.type === 'TemplateLiteral' + && node.quasis.length === 1 + && (value === undefined || node.quasis[0].value.raw === value); +} + +function getStringValue(node) { + isTemplateLiteral(node) + ? node.quasis[0].value.raw + : node.value; +} + +function getAccessorValue(accessor) { + return accessor.type === 'Identifier' + ? accessor.name + : getStringValue(accessor); +} + +function isDescribeProperty(node) { + const describeProperties = new Set(['parallel', 'serial', 'only', 'skip']); + + return isAccessor(node) && describeProperties.has(getAccessorValue(node)); +} + +function isDescribeCall(node) { + if (isDescribeAlias(node.callee)) { + return true; + } + + const callee = + node.callee.type === 'TaggedTemplateExpression' + ? node.callee.tag + : node.callee.type === 'CallExpression' + ? node.callee.callee + : node.callee; + + if(callee.type === 'MemberExpression' && isDescribeAlias(callee.property)) { + return true; + } + + if (callee.type === 'MemberExpression' && isDescribeProperty(callee.property)) { + + return callee.object.type === 'MemberExpression' + ? callee.object.object.type === 'MemberExpression' + ? isDescribeAlias(callee.object.object.property) + : isDescribeAlias(callee.object.property) + : (isDescribeAlias(callee.property) || isDescribeAlias(callee.object)); + } + + return false; +}; + module.exports = { isObject, isCalleeProperty, @@ -58,4 +138,7 @@ module.exports = { isStringLiteral, isBooleanLiteral, isBinaryExpression, + isCallExpression, + isMemberExpression, + isDescribeCall }; diff --git a/test/max-nested-describe.spec.js b/test/max-nested-describe.spec.js new file mode 100644 index 0000000..720bdbb --- /dev/null +++ b/test/max-nested-describe.spec.js @@ -0,0 +1,208 @@ +const { runRuleTester } = require('../lib/utils/rule-tester'); +const rule = require('../lib/rules/max-nested-describe'); + +const invalid = (code, options, errors) => ({ + code, + options: options || [], + errors: errors || [ + { + messageId: 'exceededMaxDepth', + }, + ], +}); + +const valid = (code, options) => ({ + code, + options: options || [] +}); + +runRuleTester('max-nested-describe', rule, { + invalid: [ + invalid(` + test.describe('foo', function() { + test.describe('bar', function () { + test.describe('baz', function () { + test.describe('qux', function () { + test.describe('quxx', function () { + test.describe('over limit', function () { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + }); + }); + }); + }); + }); + `), + invalid(` + describe('foo', function() { + describe('bar', function () { + describe('baz', function () { + describe('qux', function () { + describe('quxx', function () { + describe('over limit', function () { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + }); + }); + }); + }); + }); + `), + invalid(` + test.describe('foo', () => { + test.describe('bar', () => { + test.describe('baz', () => { + test.describe('baz1', () => { + test.describe('baz2', () => { + test.describe('baz3', () => { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + + test.describe('baz4', () => { + it('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + }); + }); + }); + + test.describe('qux', function () { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + }) + }); + `, [{ max: 5 }], [ + { messageId: 'exceededMaxDepth' }, + { messageId: 'exceededMaxDepth' }, + ]), + invalid(` + test.describe.only('foo', function() { + test.describe('bar', function() { + test.describe('baz', function() { + test.describe('qux', function() { + test.describe('quux', function() { + test.describe('quuz', function() { + }); + }); + }); + }); + }); + }); + `), + invalid(` + test.describe.serial.only('foo', function() { + test.describe('bar', function() { + test.describe('baz', function() { + test.describe('qux', function() { + test.describe('quux', function() { + test.describe('quuz', function() { + }); + }); + }); + }); + }); + }); + `), + invalid(` + test.describe('qux', () => { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + `, [{ max: 0 }]), + invalid(` + test.describe('foo', () => { + test.describe('bar', () => { + test.describe('baz', () => { + test("test1", () => { + expect(true).toBe(true); + }); + test("test2", () => { + expect(true).toBe(true); + }); + }); + }); + }); + `, [{ max: 2 }]), + ], + valid: [ + 'test.describe("describe tests", () => {});', + 'test.describe.only("describe focus tests", () => {});', + 'test.describe.serial.only("describe serial focus tests", () => {});', + valid(` + test('foo', function () { + expect(true).toBe(true); + }); + test('bar', () => { + expect(true).toBe(true); + }); + `, [{ max: 0 }]), + valid(` + test.describe('foo', function() { + test.describe('bar', function () { + test.describe('baz', function () { + test.describe('qux', function () { + test.describe('quxx', function () { + test('should get something', () => { + expect(getSomething()).toBe('Something'); + }); + }); + }); + }); + }); + }); + `), + valid(` + test.describe('foo', () => { + test.describe('bar', () => { + test.describe('baz', () => { + test.describe('qux', () => { + test('foo', () => { + expect(someCall().property).toBe(true); + }); + test('bar', () => { + expect(universe.answer).toBe(42); + }); + }); + test.describe('quxx', () => { + test('baz', () => { + expect(2 + 2).toEqual(4); + }); + }); + }); + }); + }); + `, [{ max: 4 }]), + valid(` + test.describe('foo', () => { + test.describe.only('bar', () => { + test.describe.skip('baz', () => { + test('something', async () => { + expect('something').toBe('something'); + }); + }); + }); + }); + `, [{ max: 3 }]), + valid(` + describe('foo', () => { + describe.only('bar', () => { + describe.skip('baz', () => { + test('something', async () => { + expect('something').toBe('something'); + }); + }); + }); + }); + `, [{ max: 3 }]) + ] +}); From 370a112cddc9eae3ba008f49a259eb593ae5e000 Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Tue, 7 Jun 2022 14:40:21 -0700 Subject: [PATCH 2/5] Export the new rule --- lib/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/index.js b/lib/index.js index 2e7aba5..cb9bf8c 100644 --- a/lib/index.js +++ b/lib/index.js @@ -6,6 +6,7 @@ const noFocusedTest = require("./rules/no-focused-test"); const noSkippedTest = require("./rules/no-skipped-test"); const noWaitForTimeout = require("./rules/no-wait-for-timeout"); const noForceOption = require("./rules/no-force-option"); +const maxNestedDescribe = require("./rules/max-nested-describe"); module.exports = { configs: { @@ -24,6 +25,7 @@ module.exports = { "playwright/no-skipped-test": "warn", "playwright/no-wait-for-timeout": "warn", "playwright/no-force-option": "warn", + "playwright/max-nested-describe": "warn", }, }, "jest-playwright": { @@ -68,5 +70,6 @@ module.exports = { "no-skipped-test": noSkippedTest, "no-wait-for-timeout": noWaitForTimeout, "no-force-option": noForceOption, + "max-nested-describe": maxNestedDescribe, }, }; From dc7e088b940601446d982abdabedb014246407fb Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Tue, 7 Jun 2022 14:40:50 -0700 Subject: [PATCH 3/5] Add docs --- README.md | 49 ++++++++++++++++++++++++++++++++ lib/rules/max-nested-describe.js | 3 +- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index dd1b088..e8f3a71 100644 --- a/README.md +++ b/README.md @@ -270,3 +270,52 @@ await page.locator('check').check(); await page.locator('input').fill('something'); ``` + +### `max-nested-describe` + +Enforces a maximum depth to nested `.describe()` calls. Useful for improving readability and parallelization of tests. + +Uses a default max depth option of `{ "max": 5 }`. + +Examples of **incorrect** code for this rule (using defaults): + +```js +test.describe('level 1', () => { + test.describe('level 2', () => { + test.describe('level 3', () => { + test.describe('level 4', () => { + test.describe('level 5', () => { + test.describe('level 6', () => { + test('this test', async ({ page }) => {}); + test('that test', async ({ page }) => {}); + }); + }); + }); + }); + }); +}); +``` + +Examples of **correct** code for this rule (using defaults): + +```js +test.describe('first level', () => { + test.describe('second level', () => { + test('this test', async ({ page }) => {}); + test('that test', async ({ page }) => {}); + }); +}); +``` + +#### Options + +The rule accepts a non-required option to override the default maximum nested describe depth (5). + +```json +{ + "playwright/max-nested-describe": [ + "error", + { "max": 3 } + ] +} +``` diff --git a/lib/rules/max-nested-describe.js b/lib/rules/max-nested-describe.js index 87318ff..9326db3 100644 --- a/lib/rules/max-nested-describe.js +++ b/lib/rules/max-nested-describe.js @@ -47,9 +47,10 @@ module.exports = { category: 'Best Practices', description: 'Enforces a maximum depth to nested describe calls', recommended: false, + url: 'https://github.com/playwright-community/eslint-plugin-playwright#max-nested-describe', }, messages: { - exceededMaxDepth: 'Too many nested describe calls ({{ depth }}). Maximum allowed is {{ max }}.', + exceededMaxDepth: 'Maximum describe call depth exceeded ({{ depth }}). Maximum allowed is {{ max }}.', }, type: 'suggestion', fixable: 'code', From 90a65d329502f6e01de2dc59b23ce71ff24ffd98 Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Tue, 7 Jun 2022 14:43:47 -0700 Subject: [PATCH 4/5] whitespace --- lib/rules/max-nested-describe.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/max-nested-describe.js b/lib/rules/max-nested-describe.js index 9326db3..bfa69ab 100644 --- a/lib/rules/max-nested-describe.js +++ b/lib/rules/max-nested-describe.js @@ -24,7 +24,7 @@ module.exports = { messageId: 'exceededMaxDepth', data: { depth: describeCallbackStack.length, max }, }); - } + } } function popDescribeCallback(node) { From a7963ee0abecf17053662be50aab10d59147a533 Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Wed, 8 Jun 2022 09:01:20 -0700 Subject: [PATCH 5/5] Address code review comments --- lib/utils/ast.js | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/lib/utils/ast.js b/lib/utils/ast.js index 8ab50a5..fc33757 100644 --- a/lib/utils/ast.js +++ b/lib/utils/ast.js @@ -66,39 +66,13 @@ function isIdentifier(node, name) { } function isDescribeAlias(node) { - return isIdentifier(node) && node.name === 'describe'; -} - -function isAccessor(node, value) { - return isIdentifier(node, value) || isStringNode(node, value); -} - -function isStringNode(node, specifics) { - return isStringLiteral(node, specifics) || isTemplateLiteral(node, specifics); -} - -function isTemplateLiteral(node, value) { - return node.type === 'TemplateLiteral' - && node.quasis.length === 1 - && (value === undefined || node.quasis[0].value.raw === value); -} - -function getStringValue(node) { - isTemplateLiteral(node) - ? node.quasis[0].value.raw - : node.value; -} - -function getAccessorValue(accessor) { - return accessor.type === 'Identifier' - ? accessor.name - : getStringValue(accessor); + return isIdentifier(node, 'describe'); } function isDescribeProperty(node) { const describeProperties = new Set(['parallel', 'serial', 'only', 'skip']); - return isAccessor(node) && describeProperties.has(getAccessorValue(node)); + return isIdentifier(node) && describeProperties.has(node.name); } function isDescribeCall(node) {