Add no-commented-out-tests rule

README.md

@@ -122,6 +122,7 @@ command line option.\
 | ✔  |     |     | [expect-expect](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/expect-expect.md)                             | Enforce assertion to be made in a test body                       |
 | ✔  |     |     | [max-nested-describe](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/max-nested-describe.md)                 | Enforces a maximum depth to nested describe calls                 |
 | ✔  | 🔧  |     | [missing-playwright-await](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/missing-playwright-await.md)       | Enforce Playwright APIs to be awaited                             |
+|     |     |     | [no-commented-out-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-commented-out-test.md)             | Disallow commented out tests                                      |
 | ✔  |     |     | [no-conditional-in-test](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-conditional-in-test.md)           | Disallow conditional logic in tests                               |
 | ✔  |     | 💡  | [no-element-handle](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-element-handle.md)                     | Disallow usage of element handles                                 |
 | ✔  |     |     | [no-eval](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-eval.md)                                         | Disallow usage of `page.$eval()` and `page.$$eval()`              |

docs/rules/no-commented-out-tests.md

@@ -0,0 +1,54 @@
+# Disallow commented out tests (`no-commented-out-tests`)
+
+This rule raises a warning about commented out tests. It's similar to
+`no-skipped-test` rule.
+
+## Rule details
+
+The rule uses fuzzy matching to do its best to determine what constitutes a
+commented out test, checking for a presence of `test(`, `test.describe(`,
+`test.skip(`, etc. in code comments.
+
+The following patterns are considered warnings:
+
+```js
+// describe('foo', () => {});
+// test.describe('foo', () => {});
+// test('foo', () => {});
+
+// test.describe.skip('foo', () => {});
+// test.skip('foo', () => {});
+
+// test.describe['skip']('bar', () => {});
+// test['skip']('bar', () => {});
+
+/*
+test.describe('foo', () => {});
+*/
+```
+
+These patterns would not be considered warnings:
+
+```js
+describe('foo', () => {});
+test.describe('foo', () => {});
+test('foo', () => {});
+
+test.describe.only('bar', () => {});
+test.only('bar', () => {});
+
+// foo('bar', () => {});
+```
+
+### Limitations
+
+The plugin looks at the literal function names within test code, so will not
+catch more complex examples of commented out tests, such as:
+
+```js
+// const testSkip = test.skip;
+// testSkip('skipped test', () => {});
+
+// const myTest = test;
+// myTest('does not have function body');
+```

src/index.ts

@@ -2,6 +2,7 @@ import globals from 'globals';
 import expectExpect from './rules/expect-expect';
 import maxNestedDescribe from './rules/max-nested-describe';
 import missingPlaywrightAwait from './rules/missing-playwright-await';
+import noCommentedOutTests from './rules/no-commented-out-tests';
 import noConditionalInTest from './rules/no-conditional-in-test';
 import noElementHandle from './rules/no-element-handle';
 import noEval from './rules/no-eval';
@@ -38,6 +39,7 @@ const index = {
     'expect-expect': expectExpect,
     'max-nested-describe': maxNestedDescribe,
     'missing-playwright-await': missingPlaywrightAwait,
+    'no-commented-out-tests': noCommentedOutTests,
     'no-conditional-in-test': noConditionalInTest,
     'no-element-handle': noElementHandle,
     'no-eval': noEval,

src/rules/no-commented-out-tests.ts

@@ -0,0 +1,44 @@
+import { Rule } from 'eslint';
+import * as ESTree from 'estree';
+import { getTestNames } from '../utils/ast';
+
+function hasTests(context: Rule.RuleContext, node: ESTree.Comment) {
+  const testNames = getTestNames(context);
+  const names = testNames.join('|');
+  const regex = new RegExp(
+    `^\\s*(${names}|describe)(\\.\\w+|\\[['"]\\w+['"]\\])?\\s*\\(`,
+    'mu',
+  );
+  return regex.test(node.value);
+}
+
+export default {
+  create(context) {
+    function checkNode(node: ESTree.Comment) {
+      if (!hasTests(context, node)) return;
+
+      context.report({
+        messageId: 'commentedTests',
+        node: node as unknown as ESTree.Node,
+      });
+    }
+
+    return {
+      Program() {
+        context.sourceCode.getAllComments().forEach(checkNode);
+      },
+    };
+  },
+  meta: {
+    docs: {
+      category: 'Best Practices',
+      description: 'Disallow commented out tests',
+      recommended: true,
+      url: 'https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/no-commented-out-tests.md',
+    },
+    messages: {
+      commentedTests: 'Some tests seem to be commented',
+    },
+    type: 'problem',
+  },
+} as Rule.RuleModule;

src/utils/ast.ts

@@ -63,9 +63,13 @@ export function isPropertyAccessor(
   return getStringValue(node.property) === name;
 }
 
-export function isTestIdentifier(context: Rule.RuleContext, node: ESTree.Node) {
+export function getTestNames(context: Rule.RuleContext) {
   const aliases = context.settings.playwright?.globalAliases?.test ?? [];
-  const testNames = ['test', ...aliases];
+  return ['test', ...aliases];
+}
+
+export function isTestIdentifier(context: Rule.RuleContext, node: ESTree.Node) {
+  const testNames = getTestNames(context);
   const regex = new RegExp(`^(${testNames.join('|')})$`);
 
   return (

test/spec/no-commented-out-tests.spec.ts

@@ -0,0 +1,158 @@
+import dedent from 'dedent';
+import rule from '../../src/rules/no-commented-out-tests';
+import { runRuleTester } from '../utils/rule-tester';
+
+const messageId = 'commentedTests';
+
+runRuleTester('no-commented-out-tests', rule, {
+  invalid: [
+    {
+      code: '// test.describe("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// describe["skip"]("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// describe[\'skip\']("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test.only("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test["skip"]("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test.skip("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test["skip"]("foo", function () {})',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: dedent`
+        // test(
+        //   "foo", function () {}
+        // )
+      `,
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: dedent`
+        /* test
+          (
+            "foo", function () {}
+          )
+        */
+      `,
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test("has title but no callback")',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test()',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test.someNewMethodThatMightBeAddedInTheFuture()',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test["someNewMethodThatMightBeAddedInTheFuture"]()',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: '// test("has title but no callback")',
+      errors: [{ column: 1, line: 1, messageId }],
+    },
+    {
+      code: dedent`
+        foo()
+        /*
+          test.describe("has title but no callback", () => {})
+        */
+        bar()
+      `,
+      errors: [{ column: 1, line: 2, messageId }],
+    },
+    // Global aliases
+    {
+      code: '// it("foo", () => {});',
+      errors: [{ column: 1, line: 1, messageId }],
+      settings: {
+        playwright: {
+          globalAliases: { test: ['it'] },
+        },
+      },
+    },
+  ],
+  valid: [
+    '// foo("bar", function () {})',
+    'test.describe("foo", function () {})',
+    'test("foo", function () {})',
+    'test.describe.only("foo", function () {})',
+    'test.only("foo", function () {})',
+    'test.skip("foo", function () {})',
+    'test("foo", function () {})',
+    'test.only("foo", function () {})',
+    'test.concurrent("foo", function () {})',
+    'var appliedSkip = describe.skip; appliedSkip.apply(describe)',
+    'var calledSkip = test.skip; calledSkip.call(it)',
+    '({ f: function () {} }).f()',
+    '(a || b).f()',
+    'testHappensToStartWithTest()',
+    'testSomething()',
+    '// latest(dates)',
+    '// TODO: unify with Git implementation from Shipit (?)',
+    '#!/usr/bin/env node',
+    dedent`
+      import { pending } from "actions"
+
+      test("foo", () => {
+        expect(pending()).toEqual({})
+      })
+    `,
+    dedent`
+      const { pending } = require("actions")
+
+      test("foo", () => {
+        expect(pending()).toEqual({})
+      })
+    `,
+    dedent`
+      test("foo", () => {
+        const pending = getPending()
+        expect(pending()).toEqual({})
+      })
+    `,
+    dedent`
+      test("foo", () => {
+        expect(pending()).toEqual({})
+      })
+
+      function pending() {
+        return {}
+      }
+    `,
+    // Global aliases
+    {
+      code: 'it("foo", () => {});',
+      settings: {
+        playwright: {
+          globalAliases: { test: ['it'] },
+        },
+      },
+    },
+  ],
+});