Custom ESLint plugin to Skip Unit Tests if Preceded by Comment with G…

…ithub Issue Link (#2660) * ESLint: set jest and playwright no-disabled-test to off * ESLint: added .eslintrc.custom.js for no-disabled-test custom rule * ESLint: addressing semicolon and module comments in .eslintrc.custom.js from linter * ESLint: prettiered .eslintrc.custom.js * ESLint: addressed PR comments by moving no-disabled-test to packages/eslint-plugin and accounting for all types of skipped tests * ESLint: fixed imports as per linting requirements * ESLint: addressed linting whitespace error * es-lint: removed no-disabled.test.spec.ts for testing purposes * eslint: completed tests for no-disabled-test.spec.ts * eslint: added no-disabled-rule documentation * eslint: apply changes to eslintrc.js Co-authored-by: sarayourfriend <[email protected]> * eslint: convert to multiline format for multi-line comment testing Co-authored-by: sarayourfriend <[email protected]> * eslint: update no-disabled-test.md * eslint: rename noDisabledTestRule to use snakeCase Co-authored-by: sarayourfriend <[email protected]> * eslint: updated test file to test for multi-line comment * eslint: removed schema content from rule * eslint: updated import of noDisabledTestRule * Small options clean up and rename --------- Co-authored-by: Chenika Bukes <[email protected]> Co-authored-by: sarayourfriend <[email protected]>
WordPress · Aug 17, 2023 · 712b837 · 712b837
1 parent fae7f80
commit 712b837
Show file tree

Hide file tree

Showing 6 changed files with 261 additions and 0 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -196,6 +196,8 @@ module.exports = {
       rules: {
         "import/no-named-as-default-member": ["off"],
         "@intlify/vue-i18n/no-raw-text": ["off"],
+        // Superceeded by `@openverse/no-unexplained-disabled-test`
+        "jest/no-disabled-test": "off",
         "no-restricted-imports": [
           "error",
           {
@@ -222,6 +224,8 @@ module.exports = {
       rules: {
         // Enable once https://github.com/playwright-community/eslint-plugin-playwright/issues/154 is resolved
         "playwright/expect-expect": ["off"],
+        // Superceeded by `@openverse/no-unexplained-disabled-test`
+        "playwright/no-skipped-test": "off",
       },
     },
     {

diff --git a/documentation/packages/eslint_plugin/no-unexplained-disabled-test.md b/documentation/packages/eslint_plugin/no-unexplained-disabled-test.md
@@ -0,0 +1,54 @@
+# `no-unexplained-disabled-test`
+
+Ensures that disabled tests have an issue comment with a GitHub link preceding
+them.
+
+Enforces the following conventions:
+
+- Checks for disabled tests marked with functions skipped functions such as
+  `test.skip`, `test.todo`, `it.skip`, `it.each.skip`, `describe.skip`, etc.,
+- Verifies that each disabled test has an issue comment with a valid GitHub link
+  preceding it
+
+## Rule Details
+
+Examples of **incorrect** code using this rule:
+
+````{admonition} Incorrect
+:class: error
+
+```ts
+test.skip('invalid skipped test', () => {
+  // Some comments without issue link
+});
+
+it.skip('another invalid skipped test', () => {
+  // Missing issue comment
+});
+
+describe.skip('my skipped suite', () => {
+  // ... tests will be skipped
+});
+```
+````
+
+Examples of **correct** code using this rule:
+
+````{admonition} Correct
+:class: tip
+
+```ts
+// https://github.com/your-org/your-repo/issues/123
+test.skip('valid skipped test', () => {
+  /* implementation */
+});
+
+/**
+ * A skipped test with a preceding multi-line comment:
+ * https://github.com/your-org/your-repo/issues/456
+ */
+it.skip('skipped test with comments', () => {
+  /* implementation */
+});
+```
+````
diff --git a/packages/eslint-plugin/src/configs/recommended.ts b/packages/eslint-plugin/src/configs/recommended.ts
@@ -6,5 +6,6 @@ export const recommended = {
         reservedPropNames: ["width", "height"],
       },
     ],
+    "@openverse/no-unexplained-disabled-test": ["error"],
   },
 }
diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -1,5 +1,7 @@
 import { analyticsConfiguration } from "./analytics-configuration"
+import { noUnexplainedDisabledTest } from "./no-unexplained-disabled-test"
 
 export default {
   "analytics-configuration": analyticsConfiguration,
+  "no-unexplained-disabled-test": noUnexplainedDisabledTest,
 }
diff --git a/packages/eslint-plugin/src/rules/no-unexplained-disabled-test.ts b/packages/eslint-plugin/src/rules/no-unexplained-disabled-test.ts
@@ -0,0 +1,73 @@
+import { OpenverseRule } from "../utils/rule-creator"
+
+import type { TSESTree } from "@typescript-eslint/utils"
+
+type MessageIds = "missingIssueComment"
+
+const messages = {
+  missingIssueComment:
+    "Disabled tests must have an issue comment with a GitHub link preceding them.",
+} as const
+
+export const noUnexplainedDisabledTest = OpenverseRule<[], MessageIds>({
+  name: "no-unexplained-disabled-test",
+  meta: {
+    type: "problem",
+    docs: {
+      description:
+        "Disabled tests must have an issue comment with a GitHub link preceding them.",
+      recommended: "error",
+    },
+    schema: [],
+    messages,
+  },
+  defaultOptions: [],
+  create(context) {
+    const sourceCode = context.getSourceCode()
+
+    const hasIssueCommentWithLink = (
+      node: TSESTree.Node | TSESTree.Comment
+    ) => {
+      const commentsBeforeNode = sourceCode.getCommentsBefore(node)
+      for (const comment of commentsBeforeNode) {
+        if (/\bhttps:\/\/github\.com\/.*?\/issues\/\d+\b/.test(comment.value)) {
+          return true
+        }
+      }
+
+      return false
+    }
+
+    const testSkipRegex = /test\.skip\s*\(/g
+    const testSkipEachRegex = /test\.skip\.each\s*\(/g
+    const testConcurrentSkipEachRegex = /test\.concurrent\.skip\.each\s*\(/g
+    const testTodoRegex = /test\.todo\s*\(/g
+    const itSkipRegex = /it\.skip\s*\(/g
+    const itEachSkipRegex = /it\.each\.skip\s*\(/g
+    const describeSkipRegex = /describe\.skip\s*\(/g
+    const describeEachSkipRegex = /describe\.each\.skip\s*\(/g
+
+    return {
+      CallExpression(node) {
+        const nodeText = sourceCode.getText(node)
+        if (
+          testSkipRegex.test(nodeText) ||
+          testSkipEachRegex.test(nodeText) ||
+          testConcurrentSkipEachRegex.test(nodeText) ||
+          testTodoRegex.test(nodeText) ||
+          itSkipRegex.test(nodeText) ||
+          itEachSkipRegex.test(nodeText) ||
+          describeSkipRegex.test(nodeText) ||
+          describeEachSkipRegex.test(nodeText)
+        ) {
+          if (!hasIssueCommentWithLink(node)) {
+            context.report({
+              node,
+              messageId: "missingIssueComment",
+            })
+          }
+        }
+      },
+    }
+  },
+})
diff --git a/packages/eslint-plugin/test/rules/no-unexplained-disabled-test.spec.ts b/packages/eslint-plugin/test/rules/no-unexplained-disabled-test.spec.ts
@@ -0,0 +1,127 @@
+import { ESLintUtils } from "@typescript-eslint/utils"
+
+import openverseEslintPlugin from "@openverse/eslint-plugin"
+
+const tester = new ESLintUtils.RuleTester({
+  parser: "@typescript-eslint/parser",
+  rules: {
+    "@openverse/no-unexplained-disabled-test": ["error"],
+  },
+})
+
+const invalidTestCases = [
+  {
+    name: "Test with missing issue comment",
+    code: `
+      test.skip('invalid test case', () => {
+        // Some comments without issue link
+      });
+    `,
+    errors: [{ messageId: "missingIssueComment" } as const],
+  },
+  {
+    name: "Test with missing issue comment on .skip.each test",
+    code: `
+        test.skip.each([
+        [1, 1, 2],
+        [1, 2, 3],
+        [2, 1, 3],
+        ])('.add(%i, %i)', (a, b, expected) => {
+        expect(a + b).toBe(expected) // will not be run
+        })
+    `,
+    errors: [{ messageId: "missingIssueComment" } as const],
+  },
+  {
+    name: "Test with missing issue comment on test.todo",
+    code: `
+        test.todo('invalid todo test', () => {
+        // Some comments
+        })
+    `,
+    errors: [{ messageId: "missingIssueComment" } as const],
+  },
+  {
+    name: "Test with missing issue comment on describe.skip",
+    code: `
+        describe.skip('my other beverage', () => {
+        // ... will be skipped
+        })
+    `,
+    errors: [{ messageId: "missingIssueComment" } as const],
+  },
+]
+
+const validTestCases = [
+  {
+    name: "Test describe.skip with valid issue comment",
+    code: `
+        // https://github.com/WordPress/openverse/issues/2573
+        describe.skip('my other beverage', () => {
+        // ... will be skipped
+        })
+    `,
+  },
+  {
+    name: "Test .skip.each with valid issue comment",
+    code: `
+        // https://github.com/WordPress/openverse/issues/2573
+        test.skip.each([
+        [1, 1, 2],
+        [1, 2, 3],
+        [2, 1, 3],
+        ])('.add(%i, %i)', (a, b, expected) => {
+        expect(a + b).toBe(expected) // will not be run
+        })
+    `,
+  },
+  {
+    name: "Test .skip with valid issue comment",
+    code: `
+      // https://github.com/your-org/your-repo/issues/123
+      test.skip('valid test case', () => {
+        // Test implementation
+      });
+    `,
+  },
+  {
+    name: "Test with valid issue comment preceded by comment",
+    code: `
+        /* A skipped test can be precded by multi-line comments:
+           https://github.com/WordPress/openverse/issues/2573
+        */
+        describe.skip.each([
+        [1, 1, 2],
+        [1, 2, 3],
+        [2, 1, 3],
+        ])('.add(%i, %i)', (a, b, expected) => {
+        test(\`returns \${expected}\`, () => {
+            expect(a + b).toBe(expected) // will not be run
+        })
+        })
+    `,
+  },
+  {
+    name: "Test concurrent.skip with valid issue comment",
+    code: `
+        // https://github.com/WordPress/openverse/issues/2573
+        test.concurrent.skip.each([
+        [1, 1, 2],
+        [1, 2, 3],
+        [2, 1, 3],
+        ])('.add(%i, %i)', async (a, b, expected) => {
+        expect(a + b).toBe(expected) // will not be run
+        })
+    `,
+  },
+]
+
+// Run the tests
+tester.run(
+  "@openverse/no-unexplained-disabled-test",
+  openverseEslintPlugin.rules["no-unexplained-disabled-test"],
+  {
+    valid: validTestCases,
+    invalid: invalidTestCases,
+  }
+)