From c2c0721be33251573cfa0ee7e17b11ca6260ea5c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 1 Oct 2015 21:37:12 -0700 Subject: [PATCH 1/2] test: load common.js in all tests common.js contains code that detects leaked variables. In preparation for an eslint rule that will enforce loading common.js in test files, load it everywhere it can be loaded and use an `eslint-disable` comment for files that intentionally leak. --- test/addons/async-hello-world/test.js | 1 + test/addons/at-exit/test.js | 1 + test/addons/heap-profiler/test.js | 2 ++ test/addons/hello-world-function-export/test.js | 1 + test/addons/hello-world/test.js | 1 + test/addons/repl-domain-abort/test.js | 1 + test/debugger/test-debugger-repl-break-in-module.js | 1 + test/debugger/test-debugger-repl-restart.js | 1 + test/debugger/test-debugger-repl-term.js | 1 + test/debugger/test-debugger-repl.js | 1 + 10 files changed, 11 insertions(+) diff --git a/test/addons/async-hello-world/test.js b/test/addons/async-hello-world/test.js index a284a019f01a8b..14b80b711b9b35 100644 --- a/test/addons/async-hello-world/test.js +++ b/test/addons/async-hello-world/test.js @@ -1,4 +1,5 @@ 'use strict'; +require('../../common'); var assert = require('assert'); var binding = require('./build/Release/binding'); var called = false; diff --git a/test/addons/at-exit/test.js b/test/addons/at-exit/test.js index c36f59b976e983..32264a6604eeb6 100644 --- a/test/addons/at-exit/test.js +++ b/test/addons/at-exit/test.js @@ -1,2 +1,3 @@ 'use strict'; +require('../../common'); var binding = require('./build/Release/binding'); diff --git a/test/addons/heap-profiler/test.js b/test/addons/heap-profiler/test.js index a4a316fa572f9b..3fa00e9b86afad 100644 --- a/test/addons/heap-profiler/test.js +++ b/test/addons/heap-profiler/test.js @@ -1,5 +1,7 @@ 'use strict'; +require('../../common'); + const binding = require('./build/Release/binding'); // Create an AsyncWrap object. diff --git a/test/addons/hello-world-function-export/test.js b/test/addons/hello-world-function-export/test.js index 1056386583faf1..6dcdf39ce1c413 100644 --- a/test/addons/hello-world-function-export/test.js +++ b/test/addons/hello-world-function-export/test.js @@ -1,4 +1,5 @@ 'use strict'; +require('../../common'); var assert = require('assert'); var binding = require('./build/Release/binding'); assert.equal('world', binding()); diff --git a/test/addons/hello-world/test.js b/test/addons/hello-world/test.js index c0492292fd7250..5637e9e6e93a3e 100644 --- a/test/addons/hello-world/test.js +++ b/test/addons/hello-world/test.js @@ -1,4 +1,5 @@ 'use strict'; +require('../../common'); var assert = require('assert'); var binding = require('./build/Release/binding'); assert.equal('world', binding.hello()); diff --git a/test/addons/repl-domain-abort/test.js b/test/addons/repl-domain-abort/test.js index a520df03805ccb..1268b61f362eed 100644 --- a/test/addons/repl-domain-abort/test.js +++ b/test/addons/repl-domain-abort/test.js @@ -1,4 +1,5 @@ 'use strict'; +require('../../common'); var assert = require('assert'); var repl = require('repl'); var stream = require('stream'); diff --git a/test/debugger/test-debugger-repl-break-in-module.js b/test/debugger/test-debugger-repl-break-in-module.js index 995f6fe0db3a3d..e3f998eb6465d3 100644 --- a/test/debugger/test-debugger-repl-break-in-module.js +++ b/test/debugger/test-debugger-repl-break-in-module.js @@ -1,4 +1,5 @@ 'use strict'; +require('../common'); var repl = require('./helper-debugger-repl.js'); repl.startDebugger('break-in-module/main.js'); diff --git a/test/debugger/test-debugger-repl-restart.js b/test/debugger/test-debugger-repl-restart.js index d3b2c169dea0a8..584cb098bf02e6 100644 --- a/test/debugger/test-debugger-repl-restart.js +++ b/test/debugger/test-debugger-repl-restart.js @@ -1,4 +1,5 @@ 'use strict'; +require('../common'); var repl = require('./helper-debugger-repl.js'); repl.startDebugger('breakpoints.js'); diff --git a/test/debugger/test-debugger-repl-term.js b/test/debugger/test-debugger-repl-term.js index 741adf682ab1dd..6872f6ce2f9c72 100644 --- a/test/debugger/test-debugger-repl-term.js +++ b/test/debugger/test-debugger-repl-term.js @@ -1,4 +1,5 @@ 'use strict'; +require('../common'); process.env.NODE_FORCE_READLINE = 1; var repl = require('./helper-debugger-repl.js'); diff --git a/test/debugger/test-debugger-repl.js b/test/debugger/test-debugger-repl.js index 2e44da55d72859..f93570c50156ce 100644 --- a/test/debugger/test-debugger-repl.js +++ b/test/debugger/test-debugger-repl.js @@ -1,4 +1,5 @@ 'use strict'; +require('../common'); var repl = require('./helper-debugger-repl.js'); repl.startDebugger('breakpoints.js'); From 34fc281d7ee407e3a4cc66af49b72902bdb0e7e1 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 1 Oct 2015 21:38:16 -0700 Subject: [PATCH 2/2] test: make common.js mandatory via linting rule test/common.js contains code that detects global variable leaks. This eslint rule checks that a module named `common` is loaded. It is only applicable to files in the test directory. Tests that intentionally leak variables can opt out with an eslint-disable comment. --- test/.eslintrc | 2 + test/common.js | 1 + test/parallel/test-domain-crypto.js | 2 +- .../test-regression-object-prototype.js | 2 +- test/parallel/test-repl-autolibs.js | 1 + tools/eslint-rules/required-modules.js | 104 ++++++++++++++++++ 6 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 tools/eslint-rules/required-modules.js diff --git a/test/.eslintrc b/test/.eslintrc index 2a8f61cb35f6be..5d77a660f88762 100644 --- a/test/.eslintrc +++ b/test/.eslintrc @@ -5,6 +5,8 @@ rules: no-undef: 0 ## allow global Buffer usage require-buffer: 0 + ## common module is mandatory in tests + required-modules: [2, "common"] globals: gc: false diff --git a/test/common.js b/test/common.js index 3ed7ce31d8c17b..c1c488c4ead15a 100644 --- a/test/common.js +++ b/test/common.js @@ -1,3 +1,4 @@ +/* eslint-disable required-modules */ 'use strict'; var path = require('path'); var fs = require('fs'); diff --git a/test/parallel/test-domain-crypto.js b/test/parallel/test-domain-crypto.js index 00de1aee272b7b..e76e8d08791c87 100644 --- a/test/parallel/test-domain-crypto.js +++ b/test/parallel/test-domain-crypto.js @@ -1,4 +1,4 @@ -/* eslint-disable strict */ +/* eslint-disable strict, required-modules */ try { var crypto = require('crypto'); } catch (e) { diff --git a/test/parallel/test-regression-object-prototype.js b/test/parallel/test-regression-object-prototype.js index 040e718948a581..b1411bf813687a 100644 --- a/test/parallel/test-regression-object-prototype.js +++ b/test/parallel/test-regression-object-prototype.js @@ -1,5 +1,5 @@ +/* eslint-disable required-modules */ 'use strict'; -//console.log('puts before'); Object.prototype.xadsadsdasasdxx = function() { }; diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js index 4103a19243aec5..e37f2d036ece4d 100644 --- a/test/parallel/test-repl-autolibs.js +++ b/test/parallel/test-repl-autolibs.js @@ -1,3 +1,4 @@ +/* eslint-disable required-modules */ 'use strict'; var assert = require('assert'); var util = require('util'); diff --git a/tools/eslint-rules/required-modules.js b/tools/eslint-rules/required-modules.js new file mode 100644 index 00000000000000..647726bf9a2424 --- /dev/null +++ b/tools/eslint-rules/required-modules.js @@ -0,0 +1,104 @@ +/** + * @fileoverview Require usage of specified node modules. + * @author Rich Trott + */ +'use strict'; + +var path = require('path'); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = function(context) { + // trim required module names + var requiredModules = context.options; + + var foundModules = []; + + // if no modules are required we don't need to check the CallExpressions + if (requiredModules.length === 0) { + return {}; + } + + /** + * Function to check if a node is a string literal. + * @param {ASTNode} node The node to check. + * @returns {boolean} If the node is a string literal. + */ + function isString(node) { + return node && node.type === 'Literal' && typeof node.value === 'string'; + } + + /** + * Function to check if a node is a require call. + * @param {ASTNode} node The node to check. + * @returns {boolean} If the node is a require call. + */ + function isRequireCall(node) { + return node.callee.type === 'Identifier' && node.callee.name === 'require'; + } + + /** + * Function to check if a node has an argument that is a required module and + * return its name. + * @param {ASTNode} node The node to check + * @returns {undefined|String} required module name or undefined + */ + function getRequiredModuleName(node) { + var moduleName; + + // node has arguments and first argument is string + if (node.arguments.length && isString(node.arguments[0])) { + var argValue = path.basename(node.arguments[0].value.trim()); + + // check if value is in required modules array + if (requiredModules.indexOf(argValue) !== -1) { + moduleName = argValue; + } + } + + return moduleName; + } + + return { + 'CallExpression': function(node) { + if (isRequireCall(node)) { + var requiredModuleName = getRequiredModuleName(node); + + if (requiredModuleName) { + foundModules.push(requiredModuleName); + } + } + }, + 'Program:exit': function(node) { + if (foundModules.length < requiredModules.length) { + var missingModules = requiredModules.filter( + function(module) { + return foundModules.indexOf(module === -1); + } + ); + missingModules.forEach(function(moduleName) { + context.report( + node, + 'Mandatory module "{{moduleName}}" must be loaded.', + { moduleName: moduleName } + ); + }); + } + } + }; +}; + +module.exports.schema = { + 'type': 'array', + 'items': [ + { + 'enum': [0, 1, 2] + } + ], + 'additionalItems': { + 'type': 'string' + }, + 'uniqueItems': true +};