From 70250f74e3c64741d8b9270941115fca623a4e0f Mon Sep 17 00:00:00 2001 From: John Hill Date: Thu, 13 Aug 2026 13:31:27 -0700 Subject: [PATCH 1/2] fix(no-skipped-test): detect testInfo annotations, split allowConditional `testInfo.skip()` and `testInfo.fixme()` were never reported, since the rule only looked at chains rooted at the `test` identifier. Resolve the second parameter of a test, hook, or step callback and treat annotations called on it the same as the standalone `test.skip()` form. `allowConditional` now also accepts an object so `skip` and `fixme` can be configured separately, which lets a project rely on conditional skips while still forbidding every use of `.fixme()`. A boolean keeps working and applies to both annotations. --- docs/rules/no-skipped-test.md | 50 ++++++++++- src/rules/no-skipped-test.test.ts | 132 ++++++++++++++++++++++++++++++ src/rules/no-skipped-test.ts | 123 +++++++++++++++++++++++----- 3 files changed, 284 insertions(+), 21 deletions(-) diff --git a/docs/rules/no-skipped-test.md b/docs/rules/no-skipped-test.md index 14be830..8f9b7bf 100644 --- a/docs/rules/no-skipped-test.md +++ b/docs/rules/no-skipped-test.md @@ -19,6 +19,10 @@ test.describe('skip test inside describe', () => { test.describe('skip test conditionally', async ({ browserName }) => { test.skip(browserName === 'firefox', 'Working on it') }) + +test('skip using testInfo', async ({ page }, testInfo) => { + testInfo.skip() +}) ``` With the `disallowFixme` option enabled, the following are also incorrect: @@ -29,6 +33,10 @@ test.fixme('temporarily disabled', async ({ page }) => {}) test.fixme() // marks all tests in the file as fixme test.describe.fixme('skip this describe', () => {}) + +test('fixme using testInfo', async ({ page }, testInfo) => { + testInfo.fixme() +}) ``` Examples of **correct** code for this rule: @@ -85,7 +93,47 @@ test('foo', ({ browserName }) => { }) ``` +`allowConditional` can also be an object to configure the `skip` and `fixme` +annotations separately, which is useful if you rely on conditional skips but +never want to allow `.fixme()`: + +```json +{ + "playwright/no-skipped-test": [ + "error", + { + "allowConditional": { "fixme": false, "skip": true }, + "disallowFixme": true + } + ] +} +``` + +Examples of **incorrect** code for the +`{ "allowConditional": { "skip": true }, "disallowFixme": true }` option: + +```javascript +test('foo', ({ isMobile }) => { + test.fixme(isMobile, 'Not ready for mobile yet') + expect(1).toBe(1) +}) +``` + +Example of **correct** code for the same option: + +```javascript +test('foo', ({ browserName }) => { + test.skip(browserName === 'firefox', 'Still working on it') + expect(1).toBe(1) +}) +``` + +Passing a boolean is equivalent to setting both keys to that value, so +`{ "allowConditional": true }` is the same as +`{ "allowConditional": { "fixme": true, "skip": true } }`. + ### `disallowFixme` Setting this option to `true` will also disallow the `.fixme()` annotation -(`test.fixme()`, `test.describe.fixme()`, etc.). Default is `false`. +(`test.fixme()`, `test.describe.fixme()`, `testInfo.fixme()`, etc.). Default is +`false`. diff --git a/src/rules/no-skipped-test.test.ts b/src/rules/no-skipped-test.test.ts index 9eb285b..4afc04f 100644 --- a/src/rules/no-skipped-test.test.ts +++ b/src/rules/no-skipped-test.test.ts @@ -560,6 +560,121 @@ runRuleTester('no-skipped-test', rule, { ], options: [{ disallowFixme: true }], }, + // Conditional fixme can be disallowed while conditional skip is allowed + { + code: 'test("foo", ({ isMobile }) => { test.fixme(isMobile, "Not ready") })', + errors: [ + { + column: 33, + data: { annotation: 'fixme' }, + endColumn: 66, + line: 1, + messageId: 'noSkippedTest', + suggestions: [ + { + data: { annotation: 'fixme' }, + messageId: 'removeAnnotation', + output: 'test("foo", ({ isMobile }) => { })', + }, + ], + }, + ], + options: [{ allowConditional: { skip: true }, disallowFixme: true }], + }, + // testInfo annotations + { + code: dedent` + test("foo", async ({ page }, testInfo) => { + testInfo.skip(); + }); + `, + errors: [ + { + column: 3, + data: { annotation: 'skip' }, + endColumn: 18, + line: 2, + messageId: 'noSkippedTest', + suggestions: [ + { + data: { annotation: 'skip' }, + messageId: 'removeAnnotation', + output: 'test("foo", async ({ page }, testInfo) => {\n \n});', + }, + ], + }, + ], + }, + { + code: dedent` + test("foo", async ({ page }, testInfo) => { + testInfo.skip(isMobile, "Not ready"); + }); + `, + errors: [ + { + column: 3, + data: { annotation: 'skip' }, + endColumn: 39, + line: 2, + messageId: 'noSkippedTest', + suggestions: [ + { + data: { annotation: 'skip' }, + messageId: 'removeAnnotation', + output: 'test("foo", async ({ page }, testInfo) => {\n \n});', + }, + ], + }, + ], + }, + { + code: dedent` + test("foo", async ({ page }, testInfo) => { + testInfo.fixme(isMobile, "Not ready"); + }); + `, + errors: [ + { + column: 3, + data: { annotation: 'fixme' }, + endColumn: 40, + line: 2, + messageId: 'noSkippedTest', + suggestions: [ + { + data: { annotation: 'fixme' }, + messageId: 'removeAnnotation', + output: 'test("foo", async ({ page }, testInfo) => {\n \n});', + }, + ], + }, + ], + options: [{ allowConditional: { skip: true }, disallowFixme: true }], + }, + { + code: dedent` + test.beforeEach(async ({ page }, testInfo) => { + testInfo["skip"](); + }); + `, + errors: [ + { + column: 3, + data: { annotation: 'skip' }, + endColumn: 21, + line: 2, + messageId: 'noSkippedTest', + suggestions: [ + { + data: { annotation: 'skip' }, + messageId: 'removeAnnotation', + output: 'test.beforeEach(async ({ page }, testInfo) => {\n \n});', + }, + ], + }, + ], + }, ], valid: [ 'test("a test", () => {});', @@ -606,6 +721,23 @@ runRuleTester('no-skipped-test', rule, { `, options: [{ allowConditional: true }], }, + { + code: 'test("foo", ({ browserName }) => { test.skip(browserName === "firefox", "Still working on it") })', + options: [{ allowConditional: { skip: true } }], + }, + { + code: 'test("foo", ({ isMobile }) => { test.fixme(isMobile, "Not ready") })', + options: [{ allowConditional: { fixme: true, skip: true }, disallowFixme: true }], + }, + // testInfo annotations + 'test("foo", async ({ page }, testInfo) => { testInfo.slow(); });', + 'test("foo", async ({ page }, testInfo) => { testInfo.fixme(); });', + 'test("foo", async (fixtures) => { fixtures.skip(); });', + 'notATest(async ({ page }, testInfo) => { testInfo.skip(); });', + { + code: 'test("foo", async ({ page }, testInfo) => { testInfo.skip(isMobile, "Not ready"); });', + options: [{ allowConditional: true }], + }, // Global aliases { code: 'it("a test", () => {});', diff --git a/src/rules/no-skipped-test.ts b/src/rules/no-skipped-test.ts index 8acd24a..aab375b 100644 --- a/src/rules/no-skipped-test.ts +++ b/src/rules/no-skipped-test.ts @@ -1,38 +1,107 @@ -import { findParent, getStringValue } from '../utils/ast.js' +import type { Rule, Scope } from 'eslint' +import type * as ESTree from 'estree' +import { findParent, getStringValue, isFunction } from '../utils/ast.js' import { createRule } from '../utils/createRule.js' import { parseFnCall } from '../utils/parseFnCall.js' +import type { NodeWithParent } from '../utils/types.js' + +/** + * Resolves the variable an identifier refers to, walking up the scope chain + * since the identifier may be declared in an outer scope. + */ +function resolveVariable(context: Rule.RuleContext, node: ESTree.Identifier) { + let scope: Scope.Scope | null = context.sourceCode.getScope(node as Rule.Node) + + while (scope) { + const variable = scope.variables.find((v) => v.name === node.name) + if (variable) { + return variable + } + + scope = scope.upper + } +} + +/** + * Checks if the node is the `testInfo` argument of a test or hook callback, + * e.g. the `testInfo` in `test('name', async ({ page }, testInfo) => {})`. + */ +function isTestInfo(context: Rule.RuleContext, node: ESTree.Node) { + if (node.type !== 'Identifier') { + return false + } + + const def = resolveVariable(context, node)?.defs[0] + if (def?.type !== 'Parameter') { + return false + } + + // `testInfo` is always the second argument of the callback. + const fn = def.node + if (!isFunction(fn) || fn.params[1] !== def.name) { + return false + } + + // The callback has to be an argument of a test or hook call. + const parent = (fn as NodeWithParent).parent + if (parent?.type !== 'CallExpression' || !parent.arguments.includes(fn)) { + return false + } + + const call = parseFnCall(context, parent) + return call?.group === 'test' || call?.group === 'hook' || call?.group === 'step' +} export default createRule({ create(context) { + const options = context.options[0] || {} + const disallowFixme = !!options.disallowFixme + const allowConditional = + typeof options.allowConditional === 'object' + ? { + fixme: !!options.allowConditional.fixme, + skip: !!options.allowConditional.skip, + } + : { + fixme: !!options.allowConditional, + skip: !!options.allowConditional, + } + + const isSkipAnnotation = (value: string) => + value === 'skip' || (disallowFixme && value === 'fixme') + return { CallExpression(node) { - const options = context.options[0] || {} - const allowConditional = !!options.allowConditional - const disallowFixme = !!options.disallowFixme + // If the call is a standalone `test.skip()` call, and not a test + // annotation, we have to treat it a bit differently. + let isStandalone = false + let skipNode: ESTree.Node | undefined const call = parseFnCall(context, node) - if (call?.group !== 'test' && call?.group !== 'describe' && call?.group !== 'step') { - return + if (call?.group === 'test' || call?.group === 'describe' || call?.group === 'step') { + isStandalone = call.type === 'config' + skipNode = call.members.find((member) => isSkipAnnotation(getStringValue(member))) + } else if ( + node.callee.type === 'MemberExpression' && + isSkipAnnotation(getStringValue(node.callee.property)) && + isTestInfo(context, node.callee.object) + ) { + // `testInfo.skip()` behaves the same as a standalone `test.skip()`. + isStandalone = true + skipNode = node.callee.property } - const skipNode = call.members.find((member) => { - const value = getStringValue(member) - return value === 'skip' || (disallowFixme && value === 'fixme') - }) - if (!skipNode) { return } - // If the call is a standalone `test.skip()` call, and not a test - // annotation, we have to treat it a bit differently. - const isStandalone = call.type === 'config' + const annotation = getStringValue(skipNode) // If allowConditional is enabled and it's not a test/describe function, // we ignore any `test.skip` calls that have no arguments. if ( isStandalone && - allowConditional && + allowConditional[annotation as 'fixme' | 'skip'] && (node.arguments.length !== 0 || findParent(node, 'BlockStatement')?.parent?.type === 'IfStatement' || findParent(node, 'SwitchCase') !== undefined) @@ -40,18 +109,16 @@ export default createRule({ return } - const annotation = getStringValue(skipNode) - context.report({ data: { annotation }, messageId: 'noSkippedTest', node: isStandalone ? node : skipNode, suggest: [ { - data: { annotation: getStringValue(skipNode) }, + data: { annotation }, fix: (fixer) => { return isStandalone - ? fixer.remove(node.parent) + ? fixer.remove((node as NodeWithParent).parent) : fixer.removeRange([ skipNode.range![0] - 1, skipNode.range![1] + Number(skipNode.type !== 'Identifier'), @@ -80,8 +147,24 @@ export default createRule({ additionalProperties: false, properties: { allowConditional: { + anyOf: [ + { + type: 'boolean', + }, + { + additionalProperties: false, + properties: { + fixme: { + type: 'boolean', + }, + skip: { + type: 'boolean', + }, + }, + type: 'object', + }, + ], default: false, - type: 'boolean', }, disallowFixme: { default: false, From c79cd7b4f6ed152ddfff6e1ad0dd9c3bd3da440c Mon Sep 17 00:00:00 2001 From: John Hill Date: Thu, 13 Aug 2026 15:52:58 -0700 Subject: [PATCH 2/2] docs(no-skipped-test): show the fixme-allowed allowConditional shape --- docs/rules/no-skipped-test.md | 40 +++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/docs/rules/no-skipped-test.md b/docs/rules/no-skipped-test.md index 8f9b7bf..6bd35bb 100644 --- a/docs/rules/no-skipped-test.md +++ b/docs/rules/no-skipped-test.md @@ -128,6 +128,46 @@ test('foo', ({ browserName }) => { }) ``` +The inverse is just as common, and is arguably the more useful of the two: a +team that treats `.fixme()` as documentation for a known, ticketed bug wants +conditional `.fixme()` allowed while still catching an unconditional skip that +someone left behind. + +```json +{ + "playwright/no-skipped-test": [ + "error", + { + "allowConditional": { "fixme": true, "skip": false }, + "disallowFixme": true + } + ] +} +``` + +Example of **correct** code for that option: + +```javascript +test('foo', ({ isMobile }) => { + test.fixme(isMobile, 'ref WET-204 — layout breaks below 768px') + expect(1).toBe(1) +}) +``` + +Examples of **incorrect** code for the same option: + +```javascript +// Unconditional — nothing says when this comes back +test.fixme('foo', ({}) => { + expect(1).toBe(1) +}) + +test('bar', ({ browserName }) => { + test.skip(browserName === 'firefox', 'Still working on it') + expect(1).toBe(1) +}) +``` + Passing a boolean is equivalent to setting both keys to that value, so `{ "allowConditional": true }` is the same as `{ "allowConditional": { "fixme": true, "skip": true } }`.