diff --git a/.changeset/skipped-tests-ast-detection.md b/.changeset/skipped-tests-ast-detection.md new file mode 100644 index 0000000..ce4a625 --- /dev/null +++ b/.changeset/skipped-tests-ast-detection.md @@ -0,0 +1,11 @@ +--- +'@noctcore/eslint-plugin-code-quality': patch +--- + +`skipped-tests-need-tracking` no longer reports the text of a skip that is not a call. + +**Reports less.** The rule found `it.skip(`, `test.fixme(`, `xit(`, `xdescribe(` and `xtest(` by scanning each source line, so the same text inside a string, a template literal or a comment was reported as a skipped test. It now reads the skip from the call itself. A test file that lints probe code held as a string (a config wiring test with seeded violations, a `RuleTester` suite for a wrapper rule) can spell a skipped test in its own source, and a comment can mention `it.skip(` in prose. + +Every real skip that was reported before is still reported: the `.skip` / `.fixme` modifier on `it`, `test` or `describe` (also reached through a member, `test.describe.skip(`), and the `xit` / `xdescribe` / `xtest` aliases. The tracking marker is still looked up in the source text of the lookback window, so a marker in a comment counts as before. + +The report now sits on the callee (`it.skip`, `xit`) or on the `node:test` option, where it used to cover the whole line from column 1. The line is unchanged, so an `eslint-disable-next-line` keeps working. diff --git a/packages/eslint-plugin-code-quality/docs/rules/skipped-tests-need-tracking.md b/packages/eslint-plugin-code-quality/docs/rules/skipped-tests-need-tracking.md index ad142dc..9695bd1 100644 --- a/packages/eslint-plugin-code-quality/docs/rules/skipped-tests-need-tracking.md +++ b/packages/eslint-plugin-code-quality/docs/rules/skipped-tests-need-tracking.md @@ -18,12 +18,13 @@ outright, so it can never legitimately appear with or without tracking. ## What it flags -Any line matching a skip form — `it.skip(` / `test.skip(` / `describe.skip(`, the `.fixme(` variants, -`xit(`, `xdescribe(`, `xtest(` — with **no** tracking marker on that line or within the `lookback` -window above it. +Any call of a skip form (`it.skip(` / `test.skip(` / `describe.skip(`, the `.fixme(` variants, +`xit(`, `xdescribe(`, `xtest(`) with **no** tracking marker on that line or within the `lookback` +window above it. The report sits on the callee (`it.skip`, `xit`). -Faithful to the original text-scanning implementation, the source is scanned line by line, so a marker -in a trailing comment, a preceding comment, or anywhere in the lookback window is honoured. +The skip is read from the syntax, the marker from the source text: the lines of the lookback window +are scanned as written, so a marker in a trailing comment, a preceding comment, or anywhere in the +window is honoured. ```ts bad filename=src/example.test.ts reports=2 // untracked @@ -41,9 +42,8 @@ it.skip('later', () => {}); // https://github.com/org/repo/issues/1 ### `node:test` skips -`node:test` skips through the test's options or its context, which a line scan cannot tell apart -from a platform guard. These forms are read from the syntax, and only an unconditional skip is -reported, from the line of the option or the call: +`node:test` skips through the test's options or its context, the same way it spells a platform +guard. Only an unconditional skip is reported, on the option or the call: - a `skip` or `todo` option on `test` / `it` / `describe` / `suite` (and a `t.test` subtest) whose value is a truthy literal: `{ skip: true }`, `{ skip: 1 }`, `{ todo: 'write it' }`; @@ -77,6 +77,8 @@ test('signs the release', (t) => { `t.skip(...)` inside an `if`, a loop or a helper. Those are platform or environment guards. - A `skip` / `todo` key in an object passed to anything that is not a test runner, and a `skip` method on a receiver that is not the test's context (`query.skip(10)`). +- The text of a skip that is not a call: inside a string, a template literal or a comment. A test + that lints probe code held as a string can spell a skipped test in its own source. ```ts good filename=scripts/release.test.ts test('uses POSIX signals', { skip: process.platform === 'win32' }, () => {}); @@ -89,6 +91,15 @@ test('kills the process group', (t) => { }); ``` +```ts good filename=src/example.test.ts +// A probe for the linter, e.g. it.skip('later', () => {}), is not a skipped test. +const probe = "it.skip('later', () => {});"; + +it('reports an untracked skip', async () => { + expect(await lint(probe)).toHaveLength(1); +}); +``` + ## Options | Option | Type | Default | Meaning | diff --git a/packages/eslint-plugin-code-quality/src/rules/skipped-tests-need-tracking.ts b/packages/eslint-plugin-code-quality/src/rules/skipped-tests-need-tracking.ts index 60433d6..f0e8e03 100644 --- a/packages/eslint-plugin-code-quality/src/rules/skipped-tests-need-tracking.ts +++ b/packages/eslint-plugin-code-quality/src/rules/skipped-tests-need-tracking.ts @@ -2,6 +2,7 @@ import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema'; import { createRule } from '../createRule'; +import { runnerName } from '../utils/ast'; const RULE_NAME = 'skipped-tests-need-tracking'; @@ -23,25 +24,25 @@ type MessageIds = 'needsTracking'; * human attached. `.only` is NOT listed here: `no-focused-tests` bans it * outright, so it can never legitimately appear with or without tracking. * - * Faithful to the original text-scanning implementation: the source is scanned - * line by line rather than through the AST, so a marker in a trailing or - * preceding comment (or anywhere in the lookback window) is honoured exactly as - * a human reviewer would read it. + * A skip is found in the AST, as a call, so the same text inside a string, a + * template literal or a comment is not one. Only the tracking marker is looked + * up in the source text: the lines of the lookback window are scanned as + * written, so a marker in a trailing or preceding comment (or anywhere in the + * window) is honoured exactly as a human reviewer would read it. * * `node:test` skips a test through its options (`test('x', { skip: true })`, - * `{ todo: 'reason' }`) or its context (`t.skip()`, `t.todo()`). Those are read - * from the AST, because only an UNCONDITIONAL skip is debt: `{ skip: - * process.platform === 'win32' }` or a `t.skip('POSIX only')` inside an `if` is - * a platform guard, not a test someone meant to come back to. The marker is - * looked up the same way, from the line of the option or the call. + * `{ todo: 'reason' }`) or its context (`t.skip()`, `t.todo()`). Only an + * UNCONDITIONAL skip is debt there: `{ skip: process.platform === 'win32' }` or + * a `t.skip('POSIX only')` inside an `if` is a platform guard, not a test + * someone meant to come back to. The marker is looked up the same way, from the + * line of the option or the call. */ -const SKIP_PATTERNS: readonly { pattern: RegExp; label: string }[] = [ - { pattern: /\b(?:it|test|describe)\.skip\s*\(/u, label: '.skip(' }, - { pattern: /\b(?:it|test|describe)\.fixme\s*\(/u, label: '.fixme(' }, - { pattern: /\bxit\s*\(/u, label: 'xit(' }, - { pattern: /\bxdescribe\s*\(/u, label: 'xdescribe(' }, - { pattern: /\bxtest\s*\(/u, label: 'xtest(' }, -]; +/** Runners that skip through a modifier: `it.skip(`, `test.describe.fixme(`. */ +const SKIPPABLE_RUNNERS = new Set(['it', 'test', 'describe']); +/** Modifiers that skip the runner they are called on. */ +const SKIP_MODIFIERS = new Set(['skip', 'fixme']); +/** Runner aliases that skip by name. */ +const SKIPPED_RUNNERS = new Set(['xit', 'xdescribe', 'xtest']); /** Runners whose options object may carry `skip` / `todo` (`node:test`, and `t.test` subtests). */ const NODE_TEST_RUNNERS = new Set(['test', 'it', 'describe', 'suite']); @@ -65,16 +66,32 @@ const optionSchema: JSONSchema4 = { }, }; -/** Root identifier of a test callee: `test`, `describe.skip`, `test.each(table)`. */ -function runnerName(callee: TSESTree.Node): string | null { - let current: TSESTree.Node = callee; - while ( - current.type === AST_NODE_TYPES.MemberExpression || - current.type === AST_NODE_TYPES.CallExpression - ) { - current = current.type === AST_NODE_TYPES.MemberExpression ? current.object : current.callee; +/** Last name of a callee: `xit` for `xit`, `skip` for `it.skip`, else null. */ +function calleeName(callee: TSESTree.Node): string | null { + if (callee.type === AST_NODE_TYPES.Identifier) { + return callee.name; + } + return callee.type === AST_NODE_TYPES.MemberExpression && + !callee.computed && + callee.property.type === AST_NODE_TYPES.Identifier + ? callee.property.name + : null; +} + +/** The label of a skipping runner call (`it.skip(...)`, `xit(...)`), else null. */ +function skipLabel(callee: TSESTree.Node): string | null { + const name = calleeName(callee); + if (name === null) { + return null; + } + if (SKIPPED_RUNNERS.has(name)) { + return `${name}(`; + } + if (callee.type !== AST_NODE_TYPES.MemberExpression || !SKIP_MODIFIERS.has(name)) { + return null; } - return current.type === AST_NODE_TYPES.Identifier ? current.name : null; + const runner = calleeName(callee.object); + return runner !== null && SKIPPABLE_RUNNERS.has(runner) ? `.${name}(` : null; } /** True for a call to a test runner, including a subtest (`t.test(...)`). */ @@ -170,18 +187,16 @@ export const skippedTestsNeedTrackingRule = createRule( return markers.some((marker) => marker.test(window)); } - /** Report `label` on the 1-based `line` unless a marker sits in its lookback window. */ - function checkLine(line: number, label: string): void { - const index = line - 1; + /** + * Report `label` on `node` unless a marker sits on the line `node` ends on + * or in the lookback window above it. + */ + function check(node: TSESTree.Node, label: string): void { + const index = node.loc.end.line - 1; if (hasTrackingMarker(Math.max(0, index - lookback), index)) { return; } - const text = lines[index] ?? ''; - context.report({ - loc: { start: { line, column: 0 }, end: { line, column: text.length } }, - messageId: 'needsTracking', - data: { label }, - }); + context.report({ node, messageId: 'needsTracking', data: { label } }); } return { @@ -197,12 +212,17 @@ export const skippedTestsNeedTrackingRule = createRule( } const key = propertyKey(property); if (key !== null && NODE_TEST_SKIPS.has(key) && isUnconditionalSkip(property.value)) { - checkLine(property.loc.start.line, `{ ${key} }`); + check(property, `{ ${key} }`); } } } } const callee = node.callee; + const label = skipLabel(callee); + if (label !== null) { + check(callee, label); + return; + } if ( callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed || @@ -213,29 +233,7 @@ export const skippedTestsNeedTrackingRule = createRule( return; } if (unconditionalContextName(node) === callee.object.name) { - checkLine(node.loc.start.line, `${callee.object.name}.${callee.property.name}(`); - } - }, - Program(): void { - for (let i = 0; i < lines.length; i++) { - const line = lines[i] ?? ''; - for (const { pattern, label } of SKIP_PATTERNS) { - if (!pattern.test(line)) { - continue; - } - const start = Math.max(0, i - lookback); - if (hasTrackingMarker(start, i)) { - continue; - } - context.report({ - loc: { - start: { line: i + 1, column: 0 }, - end: { line: i + 1, column: line.length }, - }, - messageId: 'needsTracking', - data: { label }, - }); - } + check(callee, `${callee.object.name}.${callee.property.name}(`); } }, }; diff --git a/packages/eslint-plugin-code-quality/tests/rules/skipped-tests-need-tracking.test.ts b/packages/eslint-plugin-code-quality/tests/rules/skipped-tests-need-tracking.test.ts index 85c253e..0d8c513 100644 --- a/packages/eslint-plugin-code-quality/tests/rules/skipped-tests-need-tracking.test.ts +++ b/packages/eslint-plugin-code-quality/tests/rules/skipped-tests-need-tracking.test.ts @@ -63,20 +63,58 @@ ruleTester.run('skipped-tests-need-tracking', skippedTestsNeedTrackingRule, { { code: "const report = pass(FILE_A, 'later', 0, { skip: true });" }, // `skip` on a non-context receiver (a query builder) is not a test skip. { code: "it('pages', (t) => { const q = query.skip(10); assert.equal(q.offset, 10); });" }, + // The text of a skip inside a string, a template literal or a comment skips nothing. + { code: "export const probe = ['it.skip(\"later\", () => {', '});'].join('\\n');" }, + { code: "export const probe = `\nit.skip('later', () => {});\nxit('later', () => {});\n`;" }, + { code: "// example: it.skip('later', () => {})\nexport {};" }, + { code: "/*\n * test.fixme('later', () => {});\n * xdescribe('later', () => {});\n */\nit('runs', () => {});" }, + { code: "it('names a probe', () => { lint(\"test('x', { skip: true }, () => {});\"); });" }, + // A reference to the skipping runner that is not called. + { code: "const maybe = ready ? it : it.skip;\nmaybe('runs', () => {});" }, + // A marker on the line the callee ends on, when the chain is split over lines. + { code: "test\n .skip('later', () => {}); // TODO(@alice): flaky under CI" }, ], invalid: [ + // The report sits on the callee, not on the whole line. { code: "it.skip('later', () => {});", - errors: [{ messageId: 'needsTracking', line: 1 }], + errors: [ + { messageId: 'needsTracking', line: 1, column: 1, endColumn: 8, data: { label: '.skip(' } }, + ], }, { code: "xdescribe('later', () => {});", - errors: [{ messageId: 'needsTracking', line: 1 }], + errors: [ + { messageId: 'needsTracking', line: 1, column: 1, endColumn: 10, data: { label: 'xdescribe(' } }, + ], }, { code: "test.fixme('later', () => {});", + errors: [{ messageId: 'needsTracking', line: 1, data: { label: '.fixme(' } }], + }, + { + code: "xtest('later', () => {});", + errors: [{ messageId: 'needsTracking', line: 1, data: { label: 'xtest(' } }], + }, + // A skip nested in a suite, a runner reached through a member, and a space before the call. + { + code: "describe('suite', () => {\n it.skip('later', () => {});\n});", + errors: [{ messageId: 'needsTracking', line: 2, column: 3 }], + }, + { + code: "test.describe.skip('later', () => {});", + errors: [{ messageId: 'needsTracking', line: 1, data: { label: '.skip(' } }], + }, + { + code: "describe.skip ('later', () => {});", errors: [{ messageId: 'needsTracking', line: 1 }], }, + // A real skip next to the same text in a string and a comment is reported once. + { + code: "// it.skip('a', () => {})\nconst probe = \"it.skip('b', () => {})\";\nit.skip('c', () => {});", + options: [{ lookback: 0 }], + errors: [{ messageId: 'needsTracking', line: 3, column: 1 }], + }, // A marker outside the lookback window does not count. { code: "// https://example.com/issue/9\n\n\nit.skip('later', () => {});",