tools: add eslint check for skipIfEslintMissing

Add a custom eslint rule to check for `common.skipIfEslintMissing()` to allow tests to run from source tarballs that do not include eslint. Fix up rule tests that were failing the new check. Refs: #20336 PR-URL: #20372 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · May 8, 2018 · 870ae72 · 870ae72
1 parent 8e6601a
commit 870ae72
Show file tree

Hide file tree

Showing 10 changed files with 104 additions and 8 deletions.
diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml
@@ -13,6 +13,7 @@ rules:
   node-core/prefer-common-expectserror: error
   node-core/prefer-common-mustnotcall: error
   node-core/crypto-check: error
+  node-core/eslint-check: error
   node-core/inspector-check: error
   node-core/number-isnan: error
   ## common module is mandatory in tests

diff --git a/test/common/index.js b/test/common/index.js
@@ -494,7 +494,7 @@ exports.fileExists = function(pathname) {
 
 exports.skipIfEslintMissing = function() {
   if (!exports.fileExists(
-    path.join('..', '..', 'tools', 'node_modules', 'eslint')
+    path.join(__dirname, '..', '..', 'tools', 'node_modules', 'eslint')
   )) {
     exports.skip('missing ESLint');
   }

diff --git a/test/parallel/test-eslint-alphabetize-errors.js b/test/parallel/test-eslint-alphabetize-errors.js
@@ -1,6 +1,7 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
+common.skipIfEslintMissing();
 
 const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
 const rule = require('../../tools/eslint-rules/alphabetize-errors');

diff --git a/test/parallel/test-eslint-buffer-constructor.js b/test/parallel/test-eslint-buffer-constructor.js
@@ -1,6 +1,7 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
+common.skipIfEslintMissing();
 
 const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
 const rule = require('../../tools/eslint-rules/buffer-constructor');

diff --git a/test/parallel/test-eslint-documented-errors.js b/test/parallel/test-eslint-documented-errors.js
@@ -1,6 +1,7 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
+common.skipIfEslintMissing();
 
 const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
 const rule = require('../../tools/eslint-rules/documented-errors');

diff --git a/test/parallel/test-eslint-eslint-check.js b/test/parallel/test-eslint-eslint-check.js
@@ -0,0 +1,31 @@
+'use strict';
+
+const common = require('../common');
+
+common.skipIfEslintMissing();
+
+const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
+const rule = require('../../tools/eslint-rules/eslint-check');
+
+const message = 'Please add a skipIfEslintMissing() call to allow this ' +
+                'test to be skipped when Node.js is built ' +
+                'from a source tarball.';
+
+new RuleTester().run('eslint-check', rule, {
+  valid: [
+    'foo;',
+    'require("common")\n' +
+      'common.skipIfEslintMissing();\n' +
+      'require("../../tools/node_modules/eslint")'
+  ],
+  invalid: [
+    {
+      code: 'require("common")\n' +
+            'require("../../tools/node_modules/eslint").RuleTester',
+      errors: [{ message }],
+      output: 'require("common")\n' +
+              'common.skipIfEslintMissing();\n' +
+              'require("../../tools/node_modules/eslint").RuleTester'
+    }
+  ]
+});
diff --git a/test/parallel/test-eslint-inspector-check.js b/test/parallel/test-eslint-inspector-check.js
@@ -1,12 +1,13 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
+common.skipIfEslintMissing();
 
 const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
 const rule = require('../../tools/eslint-rules/inspector-check');
 
 const message = 'Please add a skipIfInspectorDisabled() call to allow this ' +
-                'test to be skippped when Node is built ' +
+                'test to be skipped when Node is built ' +
                 '\'--without-inspector\'.';
 
 new RuleTester().run('inspector-check', rule, {

diff --git a/test/parallel/test-eslint-require-buffer.js b/test/parallel/test-eslint-require-buffer.js
@@ -11,7 +11,7 @@ const ruleTester = new RuleTester({
   env: { node: true }
 });
 
-const message = "Use const Buffer = require('buffer').Buffer; " +
+const message = "Use const { Buffer } = require('buffer'); " +
                 'at the beginning of this file';
 
 const useStrict = '\'use strict\';\n\n';

diff --git a/tools/eslint-rules/eslint-check.js b/tools/eslint-rules/eslint-check.js
@@ -0,0 +1,60 @@
+/**
+ * @fileoverview Check that common.skipIfEslintMissing is used if
+ *               the eslint module is required.
+ */
+'use strict';
+
+const utils = require('./rules-utils.js');
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+const msg = 'Please add a skipIfEslintMissing() call to allow this test to ' +
+            'be skipped when Node.js is built from a source tarball.';
+
+module.exports = function(context) {
+  const missingCheckNodes = [];
+  var commonModuleNode = null;
+  var hasEslintCheck = false;
+
+  function testEslintUsage(context, node) {
+    if (utils.isRequired(node, ['../../tools/node_modules/eslint'])) {
+      missingCheckNodes.push(node);
+    }
+
+    if (utils.isCommonModule(node)) {
+      commonModuleNode = node;
+    }
+  }
+
+  function checkMemberExpression(context, node) {
+    if (utils.usesCommonProperty(node, ['skipIfEslintMissing'])) {
+      hasEslintCheck = true;
+    }
+  }
+
+  function reportIfMissing(context) {
+    if (!hasEslintCheck) {
+      missingCheckNodes.forEach((node) => {
+        context.report({
+          node,
+          message: msg,
+          fix: (fixer) => {
+            if (commonModuleNode) {
+              return fixer.insertTextAfter(
+                commonModuleNode,
+                '\ncommon.skipIfEslintMissing();'
+              );
+            }
+          }
+        });
+      });
+    }
+  }
+
+  return {
+    'CallExpression': (node) => testEslintUsage(context, node),
+    'MemberExpression': (node) => checkMemberExpression(context, node),
+    'Program:exit': (node) => reportIfMissing(context, node)
+  };
+};
diff --git a/tools/eslint-rules/inspector-check.js b/tools/eslint-rules/inspector-check.js
@@ -11,7 +11,7 @@ const utils = require('./rules-utils.js');
 // Rule Definition
 //------------------------------------------------------------------------------
 const msg = 'Please add a skipIfInspectorDisabled() call to allow this ' +
-            'test to be skippped when Node is built \'--without-inspector\'.';
+            'test to be skipped when Node is built \'--without-inspector\'.';
 
 module.exports = function(context) {
   const missingCheckNodes = [];