Skip to content

Commit

Permalink
tools: add eslint check for skipIfEslintMissing
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
richardlau authored and MylesBorins committed May 8, 2018
1 parent 4b2e886 commit f17a711
Show file tree
Hide file tree
Showing 10 changed files with 104 additions and 8 deletions.
1 change: 1 addition & 0 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-eslint-alphabetize-errors.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-eslint-buffer-constructor.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-eslint-documented-errors.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-eslint-eslint-check.js
Original file line number Diff line number Diff line change
@@ -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'
}
]
});
5 changes: 3 additions & 2 deletions test/parallel/test-eslint-inspector-check.js
Original file line number Diff line number Diff line change
@@ -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, {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-eslint-require-buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
60 changes: 60 additions & 0 deletions tools/eslint-rules/eslint-check.js
Original file line number Diff line number Diff line change
@@ -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)
};
};
2 changes: 1 addition & 1 deletion tools/eslint-rules/inspector-check.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Expand Down

0 comments on commit f17a711

Please sign in to comment.