Bug 1461997 - Create a rule to prevent usages of Assert.throws/rejects without an 'expected' argument. r?Mossop draft
authorMark Banner <standard8@mozilla.com>
Mon, 09 Apr 2018 18:54:13 +0100
changeset 796211 88852452b13c8cfa655d0f54fb08f7cbcff50e2d
parent 796087 1800b8895c08bc0c60302775dc0a4b5ea4deb310
child 796212 f4f2b000a6a8cea67da0ac1ffe9ccccc340189e2
push id110181
push userbmo:standard8@mozilla.com
push dateThu, 17 May 2018 08:34:08 +0000
reviewersMossop
bugs1461997
milestone62.0a1
Bug 1461997 - Create a rule to prevent usages of Assert.throws/rejects without an 'expected' argument. r?Mossop MozReview-Commit-ID: 979uJQUjybl
.eslintrc.js
tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/require-expected-throws-or-rejects.js
tools/lint/eslint/eslint-plugin-mozilla/tests/require-expected-throws-or-rejects.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -43,10 +43,29 @@ module.exports = {
     // XXX Bug 1436303. These directories are still being fixed, so turn off
     // mozilla/no-cc-etc for now.
     "files": [
       "devtools/**"
     ],
     "rules": {
       "mozilla/no-define-cc-etc": "off",
     }
+  }, {
+    // XXX Bug 1452706. These directories are still being fixed, so turn off
+    //  mozilla/require-expected-throws-or-rejects for now.
+    "files": [
+      "browser/components/**",
+      "browser/extensions/formautofill/test/unit/test_storage_tombstones.js",
+      "browser/modules/test/browser/**",
+      "browser/tools/mozscreenshots/browser_boundingbox.js",
+      "devtools/client/inspector/extensions/test/head_devtools_inspector_sidebar.js",
+      "services/**",
+      "storage/test/unit/**",
+      "testing/marionette/test/unit/**",
+      "toolkit/components/**",
+      "toolkit/modules/tests/xpcshell/**",
+      "toolkit/mozapps/extensions/test/xpcshell/**"
+    ],
+    "rules": {
+      "mozilla/require-expected-throws-or-rejects": "off",
+    }
   }]
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
@@ -171,16 +171,17 @@ module.exports = {
     "mozilla/avoid-removeChild": "error",
     "mozilla/import-browser-window-globals": "error",
     "mozilla/import-globals": "error",
     "mozilla/no-compare-against-boolean-literals": "error",
     "mozilla/no-define-cc-etc": "error",
     "mozilla/no-import-into-var-and-global": "error",
     "mozilla/no-useless-parameters": "error",
     "mozilla/no-useless-removeEventListener": "error",
+    "mozilla/require-expected-throws-or-rejects": "error",
     "mozilla/use-cc-etc": "error",
     "mozilla/use-chromeutils-generateqi": "error",
     "mozilla/use-chromeutils-import": "error",
     "mozilla/use-default-preference-values": "error",
     "mozilla/use-includes-instead-of-indexOf": "error",
     "mozilla/use-ownerGlobal": "error",
     "mozilla/use-services": "error",
 
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -49,16 +49,18 @@ module.exports = {
     "no-import-into-var-and-global":
       require("../lib/rules/no-import-into-var-and-global.js"),
     "no-task": require("../lib/rules/no-task"),
     "no-useless-parameters": require("../lib/rules/no-useless-parameters"),
     "no-useless-removeEventListener":
       require("../lib/rules/no-useless-removeEventListener"),
     "no-useless-run-test":
       require("../lib/rules/no-useless-run-test"),
+    "require-expected-throws-or-rejects":
+      require("../lib/rules/require-expected-throws-or-rejects"),
     "reject-importGlobalProperties":
       require("../lib/rules/reject-importGlobalProperties"),
     "reject-some-requires": require("../lib/rules/reject-some-requires"),
     "use-cc-etc": require("../lib/rules/use-cc-etc"),
     "use-chromeutils-generateqi": require("../lib/rules/use-chromeutils-generateqi"),
     "use-chromeutils-import": require("../lib/rules/use-chromeutils-import"),
     "use-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/require-expected-throws-or-rejects.js
@@ -0,0 +1,70 @@
+/**
+ * @fileoverview Reject use of Cu.importGlobalProperties
+ *
+ * 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";
+
+function checkArgs(context, node, name) {
+  if (node.arguments.length < 2) {
+    context.report({
+      node,
+      messageId: "needsTwoArguments",
+      data: {
+        name
+      }
+    });
+    return;
+  }
+
+  if (!("regex" in node.arguments[1]) &&
+      node.arguments[1].type !== "FunctionExpression" &&
+      node.arguments[1].type !== "ArrowFunctionExpression" &&
+      (node.arguments[1].type !== "Identifier" ||
+       node.arguments[1].name === "undefined")) {
+    context.report({
+      node,
+      messageId: "requireExpected",
+      data: {
+        name
+      }
+    });
+  }
+}
+
+module.exports = {
+  meta: {
+    messages: {
+      needsTwoArguments: "Assert.{{name}} should have at least two arguments (assert, expected).",
+      requireExpected: "Second argument to Assert.{{name}} should be a RegExp, function or object to compare the exception to."
+    }
+  },
+
+  create(context) {
+    return {
+      "CallExpression": function(node) {
+        if (node.callee.type === "MemberExpression") {
+          let memexp = node.callee;
+          if (memexp.object.type === "Identifier" &&
+              memexp.object.name === "Assert" &&
+              memexp.property.type === "Identifier" &&
+              memexp.property.name === "rejects") {
+            // We have ourselves an Assert.rejects.
+            checkArgs(context, node, "rejects");
+          }
+
+          if (memexp.object.type === "Identifier" &&
+              memexp.object.name === "Assert" &&
+              memexp.property.type === "Identifier" &&
+              memexp.property.name === "throws") {
+            // We have ourselves an Assert.throws.
+            checkArgs(context, node, "throws");
+          }
+        }
+      }
+    };
+  }
+};
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/require-expected-throws-or-rejects.js
@@ -0,0 +1,62 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/require-expected-throws-or-rejects");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, messageId, name) {
+  return {code, errors: [{messageId, data: {name}}]};
+}
+
+ruleTester.run("no-useless-run-test", rule, {
+  valid: [
+    "Assert.throws(() => foo(), /assertion/);",
+    "Assert.throws(() => foo(), /assertion/, 'message');",
+    "Assert.throws(() => foo(), ex => {}, 'message');",
+    "Assert.throws(() => foo(), foo, 'message');",
+    "Assert.rejects(foo, /assertion/)",
+    "Assert.rejects(foo, /assertion/, 'msg')",
+    "Assert.rejects(foo, ex => {}, 'msg')",
+    "Assert.rejects(foo, foo, 'msg')"
+  ],
+  invalid: [
+    invalidCode("Assert.throws(() => foo());",
+                "needsTwoArguments", "throws"),
+
+    invalidCode("Assert.throws(() => foo(), 'message');",
+                "requireExpected", "throws"),
+
+    invalidCode("Assert.throws(() => foo(), 'invalid', 'message');",
+                "requireExpected", "throws"),
+
+    invalidCode("Assert.throws(() => foo(), null, 'message');",
+                "requireExpected", "throws"),
+
+    invalidCode("Assert.throws(() => foo(), undefined, 'message');",
+                "requireExpected", "throws"),
+
+    invalidCode("Assert.rejects(foo)",
+                "needsTwoArguments", "rejects"),
+
+    invalidCode("Assert.rejects(foo, 'msg')",
+                "requireExpected", "rejects"),
+
+    invalidCode("Assert.rejects(foo, 'invalid', 'msg')",
+                "requireExpected", "rejects"),
+
+    invalidCode("Assert.rejects(foo, undefined, 'msg')",
+                "requireExpected", "rejects")
+  ]
+});