fix(code-quality): stop skipped-tests-need-tracking reporting skips in strings and comments

.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.

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 |

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<RuleOptions, MessageIds>(
       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<RuleOptions, MessageIds>(
               }
               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<RuleOptions, MessageIds>(
           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}(`);
         }
       },
     };

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', () => {});",