Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions. r=mossop
authorMark Banner <standard8@mozilla.com>
Fri, 18 Aug 2017 21:49:44 +0100
changeset 425753 fde66058d330509d90dd4401383dbfab46a05174
parent 425752 def77bfc578f3adf963cbe2a3d2133e3d11adffd
child 425754 2dd4fb2e079a708425cb1b9509660f13fb603a34
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmossop
bugs1392098
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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();
+`)
+  ]
+});