From 3e99e55f1907a6a4f4a60142f71ce53c7cb00838 Mon Sep 17 00:00:00 2001 From: Mark Skelton Date: Tue, 27 Feb 2024 07:10:01 -0600 Subject: [PATCH] fix(valid-title): Fix more false positives and reduce future chance of error --- src/rules/no-conditional-in-test.ts | 18 +- src/rules/no-skipped-test.ts | 4 +- src/rules/require-top-level-describe.test.ts | 12 + src/rules/valid-title.test.ts | 4 + src/rules/valid-title.ts | 9 +- src/utils/parseFnCall.test.ts | 444 ++++++++++++++++--- src/utils/parseFnCall.ts | 29 +- 7 files changed, 441 insertions(+), 79 deletions(-) diff --git a/src/rules/no-conditional-in-test.ts b/src/rules/no-conditional-in-test.ts index b9c64ad..400160e 100644 --- a/src/rules/no-conditional-in-test.ts +++ b/src/rules/no-conditional-in-test.ts @@ -1,6 +1,6 @@ import { Rule } from 'eslint'; -import { findParent, getStringValue } from '../utils/ast'; -import { parseFnCall } from '../utils/parseFnCall'; +import { findParent } from '../utils/ast'; +import { isTypeOfFnCall } from '../utils/parseFnCall'; export default { create(context) { @@ -8,19 +8,7 @@ export default { const call = findParent(node, 'CallExpression'); if (!call) return; - const fnCall = parseFnCall(context, call); - - // Allow conditional logic in `test.skip()` calls inside of tests - // https://playwright.dev/docs/api/class-test#test-skip-3 - if ( - fnCall?.type === 'test' && - fnCall.members.some((member) => getStringValue(member) === 'skip') && - call.arguments[0]?.type === 'LogicalExpression' - ) { - return; - } - - if (fnCall?.type === 'test' || fnCall?.type === 'step') { + if (isTypeOfFnCall(context, call, ['test', 'step'])) { context.report({ messageId: 'conditionalInTest', node }); } } diff --git a/src/rules/no-skipped-test.ts b/src/rules/no-skipped-test.ts index edd398b..cc9fe48 100644 --- a/src/rules/no-skipped-test.ts +++ b/src/rules/no-skipped-test.ts @@ -9,7 +9,9 @@ export default { const options = context.options[0] || {}; const allowConditional = !!options.allowConditional; - const call = parseFnCall(context, node); + const call = parseFnCall(context, node, { + includeConfigStatements: true, + }); if (call?.type !== 'test' && call?.type !== 'describe') { return; } diff --git a/src/rules/require-top-level-describe.test.ts b/src/rules/require-top-level-describe.test.ts index 2358ae6..d2afa06 100644 --- a/src/rules/require-top-level-describe.test.ts +++ b/src/rules/require-top-level-describe.test.ts @@ -177,6 +177,18 @@ runRuleTester('require-top-level-describe', rule, { valid: [ 'foo()', 'test.info()', + 'test.use({ locale: "en-US" })', + // 'test.only("that all is as it should be", () => {});', + // 'test.skip("that all is as it should be", () => {});', + // 'test.skip(({ browserName }) => browserName === "Chrome");', + // 'test.skip();', + // 'test.skip(true);', + // 'test.skip(browserName === "Chrome", "This feature is skipped on Chrome")', + // 'test.slow("that all is as it should be", () => {});', + // 'test.slow(({ browserName }) => browserName === "Chrome");', + // 'test.slow();', + // 'test.slow(browserName === "webkit", "This feature is slow on Mac")', + 'test.describe.configure({ mode: "parallel" })', 'test.describe("suite", () => { test("foo") });', 'test.describe.only("suite", () => { test.beforeAll("my beforeAll") });', 'test.describe.parallel("suite", () => { test.beforeEach("my beforeAll") });', diff --git a/src/rules/valid-title.test.ts b/src/rules/valid-title.test.ts index db5a2e0..bfa266a 100644 --- a/src/rules/valid-title.test.ts +++ b/src/rules/valid-title.test.ts @@ -92,8 +92,12 @@ runRuleTester('valid-title', rule, { 'test.skip("that all is as it should be", () => {});', 'test.skip(({ browserName }) => browserName === "Chrome");', 'test.skip();', + 'test.skip(true);', + 'test.slow(true, "Always");', 'test.skip(browserName === "Chrome", "This feature is skipped on Chrome")', 'test.slow("that all is as it should be", () => {});', + 'test.slow(true);', + 'test.slow(true, "Always");', 'test.slow(({ browserName }) => browserName === "Chrome");', 'test.slow();', 'test.slow(browserName === "webkit", "This feature is slow on Mac")', diff --git a/src/rules/valid-title.ts b/src/rules/valid-title.ts index 357369e..d5c8031 100644 --- a/src/rules/valid-title.ts +++ b/src/rules/valid-title.ts @@ -110,15 +110,8 @@ export default { return; } - // Ignore statements such as `test.slow()`, `test.describe.configure()`, etc. - if (node.arguments.length < 2) { - return; - } - const [argument] = node.arguments; - if (!argument) { - return; - } + if (!argument) return; if (!isStringNode(argument)) { if ( diff --git a/src/utils/parseFnCall.test.ts b/src/utils/parseFnCall.test.ts index f86c72f..2a9e744 100644 --- a/src/utils/parseFnCall.test.ts +++ b/src/utils/parseFnCall.test.ts @@ -4,9 +4,10 @@ import * as ESTree from 'estree'; import { getStringValue } from './ast'; import { isSupportedAccessor, - ParsedFnCall, + type ParsedFnCall, + type ParseFnCallOptions, parseFnCallWithReason, - ResolvedFnWithNode, + type ResolvedFnWithNode, } from './parseFnCall'; import { runRuleTester, runTSRuleTester } from './rule-tester'; @@ -18,55 +19,57 @@ const isNode = (obj: unknown): obj is ESTree.Node => { return false; }; -const rule = { - create: (context) => ({ - CallExpression(node) { - const call = parseFnCallWithReason(context, node); +function rule(options?: ParseFnCallOptions) { + return { + create: (context) => ({ + CallExpression(node) { + const call = parseFnCallWithReason(context, node, options); - if (typeof call === 'string') { - context.report({ messageId: call, node }); - } else if (call) { - const sorted = sortKeys({ - ...call, - head: sortKeys(call.head), - }); + if (typeof call === 'string') { + context.report({ messageId: call, node }); + } else if (call) { + const sorted = sortKeys({ + ...call, + head: sortKeys(call.head), + }); - context.report({ - data: { - data: JSON.stringify(sortKeys(sorted), (_key, value) => { - if (isNode(value)) { - if (isSupportedAccessor(value)) { - return getStringValue(value); - } + context.report({ + data: { + data: JSON.stringify(sortKeys(sorted), (_key, value) => { + if (isNode(value)) { + if (isSupportedAccessor(value)) { + return getStringValue(value); + } - return undefined; - } + return undefined; + } - return value; - }), - }, - messageId: 'details', - node, - }); - } + return value; + }), + }, + messageId: 'details', + node, + }); + } + }, + }), + meta: { + docs: { + category: 'Possible Errors', + description: 'Fake rule for testing parseFnCall', + recommended: false, + }, + messages: { + details: '{{ data }}', + 'matcher-not-called': 'matcherNotCalled', + 'matcher-not-found': 'matcherNotFound', + 'modifier-unknown': 'modifierUnknown', + }, + schema: [], + type: 'problem', }, - }), - meta: { - docs: { - category: 'Possible Errors', - description: 'Fake rule for testing parseFnCall', - recommended: false, - }, - messages: { - details: '{{ data }}', - 'matcher-not-called': 'matcherNotCalled', - 'matcher-not-found': 'matcherNotFound', - 'modifier-unknown': 'modifierUnknown', - }, - schema: [], - type: 'problem', - }, -} as Rule.RuleModule; + } as Rule.RuleModule; +} interface TestResolvedFnWithNode extends Omit { node: string; @@ -100,7 +103,7 @@ const expectedParsedFnCallResultData = (result: TestParsedFnCall) => ({ }), }); -runRuleTester('nonexistent methods', rule, { +runRuleTester('nonexistent methods', rule(), { invalid: [], valid: [ 'describe.something()', @@ -123,7 +126,7 @@ runRuleTester('nonexistent methods', rule, { ], }); -runRuleTester('expect', rule, { +runRuleTester('expect', rule(), { invalid: [ { code: 'expect(x).toBe(y);', @@ -498,7 +501,7 @@ runRuleTester('expect', rule, { valid: [], }); -runRuleTester('test', rule, { +runRuleTester('test', rule(), { invalid: [ { code: 'test("a test", () => {});', @@ -540,6 +543,168 @@ runRuleTester('test', rule, { }, ], }, + { + code: 'test.only("a test", () => {});', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['only'], + name: 'test', + type: 'test', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.skip("a test", () => {});', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['skip'], + name: 'test', + type: 'test', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.slow("a test", () => {});', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['slow'], + name: 'test', + type: 'test', + }), + line: 1, + messageId: 'details', + }, + ], + }, + ], + valid: [ + 'test.info()', + 'test.use({ locale: "en-US" })', + // test.skip + 'test.skip();', + 'test.skip(true);', + 'test.skip(({ browserName }) => browserName === "Chrome");', + 'test.skip(browserName === "Chrome", "This feature is skipped on Chrome")', + // test.slow + 'test.slow();', + 'test.slow(true);', + 'test.slow(({ browserName }) => browserName === "Chrome");', + 'test.slow(browserName === "webkit", "This feature is slow on Mac")', + // Other functions + 'it("is a function", () => {});', + 'ByDefault.sayHello();', + ], +}); + +runRuleTester('describe', rule(), { + invalid: [ + { + code: 'describe("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'describe', + node: 'describe', + original: null, + }, + members: [], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'describe.skip("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'describe', + node: 'describe', + original: null, + }, + members: ['skip'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.describe("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['describe'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.describe.skip("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['describe', 'skip'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, { code: 'context("when there is an error", () => {})', errors: [ @@ -623,8 +788,8 @@ runRuleTester('test', rule, { }, ], valid: [ - 'it("is a function", () => {});', - 'ByDefault.sayHello();', + 'describe.configure({ mode: "parallel" })', + 'test.describe.configure({ mode: "parallel" })', { code: 'mycontext("skip this please", () => {});', settings: { playwright: { globalAliases: { describe: ['context'] } } }, @@ -632,7 +797,7 @@ runRuleTester('test', rule, { ], }); -runTSRuleTester('typescript', rule, { +runTSRuleTester('typescript', rule(), { invalid: [ { code: dedent` @@ -682,3 +847,178 @@ runTSRuleTester('typescript', rule, { 'dedent()', ], }); + +runRuleTester('includeConfigStatements', rule(), { + invalid: [ + { + code: 'test("a test", () => {});', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: [], + name: 'test', + type: 'test', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.step("a step", () => {});', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['step'], + name: 'step', + type: 'step', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'describe("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'describe', + node: 'describe', + original: null, + }, + members: [], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'describe.skip("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'describe', + node: 'describe', + original: null, + }, + members: ['skip'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.describe("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['describe'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'test.describe.skip("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'test', + node: 'test', + original: null, + }, + members: ['describe', 'skip'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + }, + { + code: 'context("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'context', + node: 'context', + original: 'describe', + }, + members: [], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + settings: { playwright: { globalAliases: { describe: ['context'] } } }, + }, + { + code: 'context.skip("when there is an error", () => {})', + errors: [ + { + column: 1, + data: expectedParsedFnCallResultData({ + head: { + local: 'context', + node: 'context', + original: 'describe', + }, + members: ['skip'], + name: 'describe', + type: 'describe', + }), + line: 1, + messageId: 'details', + }, + ], + settings: { playwright: { globalAliases: { describe: ['context'] } } }, + }, + ], + valid: [ + 'it("is a function", () => {});', + 'ByDefault.sayHello();', + { + code: 'mycontext("skip this please", () => {});', + settings: { playwright: { globalAliases: { describe: ['context'] } } }, + }, + ], +}); diff --git a/src/utils/parseFnCall.ts b/src/utils/parseFnCall.ts index 01fd077..e897309 100644 --- a/src/utils/parseFnCall.ts +++ b/src/utils/parseFnCall.ts @@ -4,6 +4,7 @@ import { findParent, getParent, getStringValue, + isFunction, isIdentifier, isStringNode, StringNode, @@ -306,7 +307,15 @@ export const findTopMostCallExpression = ( return top as ESTree.CallExpression & Rule.NodeParentExtension; }; -function parse(context: Rule.RuleContext, node: ESTree.CallExpression) { +export type ParseFnCallOptions = { + includeConfigStatements?: boolean; +}; + +function parse( + context: Rule.RuleContext, + node: ESTree.CallExpression, + options?: ParseFnCallOptions, +): ParsedFnCall | string | null { const chain = getNodeChain(node); if (!chain?.length) { @@ -335,6 +344,18 @@ function parse(context: Rule.RuleContext, node: ESTree.CallExpression) { } } + // If the call is a standalone `test.skip()`, `test.use`, `test.describe.configure()`, + // etc. call, and the `includeConfigStatements` option is not enabled, we + // should ignore it. Some rules use these statements, but not most. + if ( + (name === 'test' || name === 'describe') && + !options?.includeConfigStatements + ) { + if (node.arguments.length !== 2 || !isFunction(node.arguments[1])) { + return null; + } + } + const parsedFnCall: Omit = { head: { ...resolved, node: first }, // every member node must have a member expression as their parent @@ -397,12 +418,13 @@ const cache = new WeakMap< export function parseFnCallWithReason( context: Rule.RuleContext, node: ESTree.CallExpression, + options?: ParseFnCallOptions, ): ParsedFnCall | string | null { if (cache.has(node)) { return cache.get(node)!; } - const call = parse(context, node); + const call = parse(context, node, options); cache.set(node, call); return call; @@ -411,8 +433,9 @@ export function parseFnCallWithReason( export function parseFnCall( context: Rule.RuleContext, node: ESTree.CallExpression, + options?: ParseFnCallOptions, ): ParsedFnCall | null { - const call = parseFnCallWithReason(context, node); + const call = parseFnCallWithReason(context, node, options); return typeof call === 'string' ? null : call; }