Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions. r?Mossop draft
authorMark Banner <standard8@mozilla.com>
Fri, 18 Aug 2017 21:49:44 +0100
changeset 650040 0f854f11a376a4ef7c6d336ed68936771b458093
parent 649622 7dddbd85047c6dc73ddbe1e423cd643a217845b3
child 650041 4dce262c51ffbd7b2518acd576b7105ca6f0461b
push id75227
push userbmo:standard8@mozilla.com
push dateMon, 21 Aug 2017 16:23:40 +0000
reviewersMossop
bugs1392098
milestone57.0a1
Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions. r?Mossop MozReview-Commit-ID: pbXjamX4bk
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-run-test.js
tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/tests/no-useless-run-test.js
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -208,16 +208,23 @@ Reject common XPCOM methods called with 
 
 This option can be autofixed (``--fix``).
 
 no-useless-removeEventListener
 ------------------------------
 
 Reject calls to removeEventListener where {once: true} could be used instead.
 
+no-useless-run-test
+-------------------
+
+Designed for xpcshell-tests. Rejects definitions of ``run_test()`` where the
+function only contains a single call to ``run_next_test()``. xpcshell's head.js
+already defines a utility function so there is no need for duplication.
+
 reject-importGlobalProperties
 -----------------------------
 
 Rejects calls to ``Cu.importGlobalProperties``.  Use of this function is
 undesirable in some parts of the tree.
 
 
 reject-some-requires
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -47,16 +47,18 @@ module.exports = {
     "no-cpows-in-tests": require("../lib/rules/no-cpows-in-tests"),
     "no-single-arg-cu-import": require("../lib/rules/no-single-arg-cu-import"),
     "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"),
     "reject-importGlobalProperties":
       require("../lib/rules/reject-importGlobalProperties"),
     "reject-some-requires": require("../lib/rules/reject-some-requires"),
     "use-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
     "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level")
   },
@@ -72,16 +74,17 @@ module.exports = {
     "mark-test-function-used": "off",
     "no-aArgs": "off",
     "no-arbitrary-setTimeout": "off",
     "no-cpows-in-tests": "off",
     "no-single-arg-cu-import": "off",
     "no-import-into-var-and-global": "off",
     "no-task": "off",
     "no-useless-parameters": "off",
+    "no-useless-run-test": "off",
     "no-useless-removeEventListener": "off",
     "reject-importGlobalProperties": "off",
     "reject-some-requires": "off",
     "use-default-preference-values": "off",
     "use-ownerGlobal": "off",
     "var-only-at-top-level": "off"
   }
 };
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-run-test.js
@@ -0,0 +1,68 @@
+/**
+ * @fileoverview Reject run_test() definitions where they aren't necessary.
+ *
+ * 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 {
+    "Program > FunctionDeclaration": function(node) {
+      if (node.id.name === "run_test" &&
+          node.body.type === "BlockStatement" &&
+          node.body.body.length === 1 &&
+          node.body.body[0].type === "ExpressionStatement" &&
+          node.body.body[0].expression.type === "CallExpression" &&
+          node.body.body[0].expression.callee.name === "run_next_test") {
+        context.report({
+          node,
+          fix: fixer => {
+            let sourceCode = context.getSourceCode();
+            let startNode;
+            if (sourceCode.getCommentsBefore) {
+              // ESLint 4 has getCommentsBefore.
+              startNode = sourceCode.getCommentsBefore(node);
+            } else if (node && node.body && node.leadingComments) {
+              // This is for ESLint 3.
+              startNode = node.leadingComments;
+            }
+
+            // If we have comments, we want the start node to be the comments,
+            // rather than the token before the comments, so that we don't
+            // remove the comments - for run_test, these are likely to be useful
+            // information about the test.
+            if (startNode && startNode.length) {
+              startNode = startNode[startNode.length - 1];
+            } else {
+              startNode = sourceCode.getTokenBefore(node);
+            }
+
+            return fixer.removeRange([
+              // If there's no startNode, we fall back to zero, i.e. start of
+              // file.
+              startNode ? (startNode.end + 1) : 0,
+              // We know the function is a block and it'll end with }. Normally
+              // there's a new line after that, so just advance past it. This
+              // may be slightly not dodgy in some cases, but covers the existing
+              // cases.
+              node.end + 1
+            ]);
+          },
+          message: "Useless run_test function - only contains run_next_test; whole function can be removed"
+        });
+      }
+    }
+  };
+};
--- a/tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.4.1",
+  "version": "0.4.3",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
     "acorn": {
       "version": "5.1.1",
       "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.1.1.tgz",
       "integrity": "sha512-vOk6uEMctu0vQrvuSqFdJyqj1Q0S5VTDL79qtjo+DhRr+1mmaD+tluFSCZqhvi/JUhXSzoZN2BhtstaPEeE8cw=="
     },
--- a/tools/lint/eslint/eslint-plugin-mozilla/package.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.4.2",
+  "version": "0.4.3",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/no-useless-run-test.js
@@ -0,0 +1,114 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/no-useless-run-test");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, output) {
+  let message =
+    "Useless run_test function - only contains run_next_test; whole function can be removed";
+  return {code, output, errors: [{message, type: "FunctionDeclaration"}]};
+}
+
+ruleTester.run("no-useless-run-test", rule, {
+  valid: [
+    "function run_test() {}",
+    "function run_test() {let args = 1; run_next_test();}",
+    "function run_test() {fakeCall(); run_next_test();}"
+  ],
+  invalid: [
+    // Single-line case.
+    invalidCode(
+      "function run_test() { run_next_test(); }",
+      ""),
+    // Multiple-line cases
+    invalidCode(`
+function run_test() {
+  run_next_test();
+}`,
+      ``),
+    invalidCode(`
+foo();
+function run_test() {
+ run_next_test();
+}
+`,
+      `
+foo();
+`),
+    invalidCode(`
+foo();
+function run_test() {
+  run_next_test();
+}
+bar();
+`,
+      `
+foo();
+bar();
+`),
+    invalidCode(`
+foo();
+
+function run_test() {
+  run_next_test();
+}
+
+bar();`,
+      `
+foo();
+
+bar();`),
+    invalidCode(`
+foo();
+
+function run_test() {
+  run_next_test();
+}
+
+// A comment
+bar();
+`,
+      `
+foo();
+
+// A comment
+bar();
+`),
+    invalidCode(`
+foo();
+
+/**
+ * A useful comment.
+ */
+function run_test() {
+  run_next_test();
+}
+
+// A comment
+bar();
+`,
+      `
+foo();
+
+/**
+ * A useful comment.
+ */
+
+// A comment
+bar();
+`)
+  ]
+});