From e6d5cae88be513e1a3fd2888075d9bc1ee56c309 Mon Sep 17 00:00:00 2001
From: Nihanth Subramanya <nhnt11@gmail.com>
Date: Wed, 13 Oct 2021 21:26:40 +0000
Subject: [PATCH] Bug 1731780 - Reject .only() chained onto add_task in tests.
 r=Gijs,mythmon,Standard8

Differential Revision: https://phabricator.services.mozilla.com/D128027
---
 .../lint/linters/eslint-plugin-mozilla.rst    |  1 +
 .../reject-addtask-only.rst                   |  6 ++
 .../mochitest/chrome/test_tasks_skipall.xhtml |  1 +
 .../tests/browser/browser_tasks_skipall.js    |  2 +
 .../example/unit/test_tasks_skipall.js        |  2 +
 .../browser/browser_PreferenceExperiments.js  |  2 +
 .../lib/configs/recommended.js                |  1 +
 .../eslint/eslint-plugin-mozilla/lib/index.js |  1 +
 .../lib/rules/reject-addtask-only.js          | 48 ++++++++++++++++
 .../tests/reject-addtask-only.js              | 56 +++++++++++++++++++
 10 files changed, 120 insertions(+)
 create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-addtask-only.rst
 create mode 100644 tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-addtask-only.js
 create mode 100644 tools/lint/eslint/eslint-plugin-mozilla/tests/reject-addtask-only.js

diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst
index 924dfbb80887..bf2f209e9c9b 100644
--- a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst
+++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst
@@ -40,6 +40,7 @@ The plugin implements the following rules:
    eslint-plugin-mozilla/no-useless-parameters
    eslint-plugin-mozilla/no-useless-removeEventListener
    eslint-plugin-mozilla/no-useless-run-test
+   eslint-plugin-mozilla/reject-addtask-only
    eslint-plugin-mozilla/reject-chromeutils-import-params
    eslint-plugin-mozilla/reject-importGlobalProperties
    eslint-plugin-mozilla/reject-osfile
diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-addtask-only.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-addtask-only.rst
new file mode 100644
index 000000000000..e540b2441664
--- /dev/null
+++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-addtask-only.rst
@@ -0,0 +1,6 @@
+reject-addtask-only
+===================
+
+Designed for JavaScript tests using the add_task pattern. Rejects chaining
+.only() to an add_task() call, which is useful for local testing to run a
+single task in isolation but is easy to land into the tree by accident.
diff --git a/testing/mochitest/chrome/test_tasks_skipall.xhtml b/testing/mochitest/chrome/test_tasks_skipall.xhtml
index b0732fd41449..40a2fe1a5867 100644
--- a/testing/mochitest/chrome/test_tasks_skipall.xhtml
+++ b/testing/mochitest/chrome/test_tasks_skipall.xhtml
@@ -7,6 +7,7 @@
   <script type="application/javascript">
     <![CDATA[
 
+      /* eslint-disable mozilla/reject-addtask-only */
       // Check that we can skip all but one task by calling `only()`.
 
       add_task(async function skipMe1() {
diff --git a/testing/mochitest/tests/browser/browser_tasks_skipall.js b/testing/mochitest/tests/browser/browser_tasks_skipall.js
index 0562834751dd..13290e58bad8 100644
--- a/testing/mochitest/tests/browser/browser_tasks_skipall.js
+++ b/testing/mochitest/tests/browser/browser_tasks_skipall.js
@@ -1,5 +1,7 @@
 "use strict";
 
+/* eslint-disable mozilla/reject-addtask-only */
+
 add_task(async function skipMe1() {
   Assert.ok(false, "Not skipped after all.");
 });
diff --git a/testing/xpcshell/example/unit/test_tasks_skipall.js b/testing/xpcshell/example/unit/test_tasks_skipall.js
index 0562834751dd..13290e58bad8 100644
--- a/testing/xpcshell/example/unit/test_tasks_skipall.js
+++ b/testing/xpcshell/example/unit/test_tasks_skipall.js
@@ -1,5 +1,7 @@
 "use strict";
 
+/* eslint-disable mozilla/reject-addtask-only */
+
 add_task(async function skipMe1() {
   Assert.ok(false, "Not skipped after all.");
 });
diff --git a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
index 1c3d6ad7c340..26ed4af2593b 100644
--- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
+++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
@@ -2145,6 +2145,8 @@ decorate_task(
 
 // When a default-branch experiment starts, prefs that already exist and that
 // have user values should not be changed.
+// Bug 1735344:
+// eslint-disable-next-line mozilla/reject-addtask-only
 decorate_task(
   withMockExperiments(),
   withStub(TelemetryEnvironment, "setExperimentActive"),
diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
index dd98021e9fd2..b00bd958e413 100644
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
@@ -129,6 +129,7 @@ module.exports = {
     "mozilla/no-useless-removeEventListener": "error",
     "mozilla/prefer-boolean-length-check": "error",
     "mozilla/prefer-formatValues": "error",
+    "mozilla/reject-addtask-only": "error",
     "mozilla/reject-chromeutils-import-params": "error",
     "mozilla/reject-importGlobalProperties": ["error", "allownonwebidl"],
     "mozilla/reject-osfile": "warn",
diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
index ea3cd58a22ee..30dcc701570a 100644
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -53,6 +53,7 @@ module.exports = {
     "no-useless-run-test": require("../lib/rules/no-useless-run-test"),
     "prefer-boolean-length-check": require("../lib/rules/prefer-boolean-length-check"),
     "prefer-formatValues": require("../lib/rules/prefer-formatValues"),
+    "reject-addtask-only": require("../lib/rules/reject-addtask-only"),
     "reject-chromeutils-import-params": require("../lib/rules/reject-chromeutils-import-params"),
     "reject-importGlobalProperties": require("../lib/rules/reject-importGlobalProperties"),
     "reject-osfile": require("../lib/rules/reject-osfile"),
diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-addtask-only.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-addtask-only.js
new file mode 100644
index 000000000000..f972d7f7d223
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-addtask-only.js
@@ -0,0 +1,48 @@
+/**
+ * @fileoverview Don't allow only() in tests
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+module.exports = function(context) {
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    CallExpression(node) {
+      if (
+        node.callee.object &&
+        node.callee.object.callee &&
+        ["add_task", "decorate_task"].includes(
+          node.callee.object.callee.name
+        ) &&
+        node.callee.property &&
+        node.callee.property.name == "only"
+      ) {
+        context.report({
+          node,
+          message: `add_task(...).only() not allowed - add an exception if this is intentional`,
+          suggest: [
+            {
+              desc: "Remove only() call from task",
+              fix: fixer =>
+                fixer.replaceTextRange(
+                  [node.callee.object.range[1], node.range[1]],
+                  ""
+                ),
+            },
+          ],
+        });
+      }
+    },
+  };
+};
diff --git a/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-addtask-only.js b/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-addtask-only.js
new file mode 100644
index 000000000000..196b47f9cbca
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-addtask-only.js
@@ -0,0 +1,56 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/reject-addtask-only");
+var RuleTester = require("eslint").RuleTester;
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 8 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidError(output) {
+  let message =
+    "add_task(...).only() not allowed - add an exception if this is intentional";
+  return [
+    {
+      message,
+      type: "CallExpression",
+      suggestions: [{ desc: "Remove only() call from task", output }],
+    },
+  ];
+}
+
+ruleTester.run("reject-addtask-only", rule, {
+  valid: [
+    "add_task(foo())",
+    "add_task(foo()).skip()",
+    "add_task(function() {})",
+    "add_task(function() {}).skip()",
+  ],
+  invalid: [
+    {
+      code: "add_task(foo()).only()",
+      errors: invalidError("add_task(foo())"),
+    },
+    {
+      code: "add_task(foo()).only(bar())",
+      errors: invalidError("add_task(foo())"),
+    },
+    {
+      code: "add_task(function() {}).only()",
+      errors: invalidError("add_task(function() {})"),
+    },
+    {
+      code: "add_task(function() {}).only(bar())",
+      errors: invalidError("add_task(function() {})"),
+    },
+  ],
+});