test: remove common.globalCheck

BridgeAR · MylesBorins · commit 22f46e776681 · 2018-05-22T14:24:32.000-04:00
This flag is partially used in tests where it was not necessary and it is always possible to replace this flag with `common.allowGlobals`. This makes sure all globals are truly tested for. PR-URL: #20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
diff --git a/test/common/README.md b/test/common/README.md
@@ -150,11 +150,6 @@ Attempts to get a valid TTY file descriptor. Returns `-1` if it fails.
 
 The TTY file descriptor is assumed to be capable of being writable.
 
-### globalCheck
-* [&lt;boolean>]
-
-Set to `false` if the test should not check for global leaks.
-
 ### hasCrypto
 * [&lt;boolean>]
 
diff --git a/test/common/index.js b/test/common/index.js
@@ -366,21 +366,15 @@ function leakedGlobals() {
 }
 exports.leakedGlobals = leakedGlobals;
 
-// Turn this off if the test should not check for global leaks.
-exports.globalCheck = true;
-
 process.on('exit', function() {
- if (!exports.globalCheck) return;
 const leaked = leakedGlobals();
 if (leaked.length > 0) {
 assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
 }
 });
 
-
 const mustCallChecks = [];
 
-
 function runCallChecks(exitCode) {
 if (exitCode !== 0) return;
 
diff --git a/test/common/index.mjs b/test/common/index.mjs
@@ -63,11 +63,7 @@ export function leakedGlobals() {
 }
 }
 
-// Turn this off if the test should not check for global leaks.
-export let globalCheck = true; // eslint-disable-line
-
 process.on('exit', function() {
- if (!globalCheck) return;
 const leaked = leakedGlobals();
 if (leaked.length > 0) {
 assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
diff --git a/test/parallel/test-domain-crypto.js b/test/parallel/test-domain-crypto.js
@@ -29,7 +29,7 @@ if (!common.hasCrypto)
 const crypto = require('crypto');
 
 // Pollution of global is intentional as part of test.
-common.globalCheck = false;
+common.allowGlobals(require('domain'));
 // See https://github.com/nodejs/node/commit/d1eff9ab
 global.domain = require('domain');
 
diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js
@@ -28,7 +28,7 @@ const fixtures = require('../common/fixtures');
 
 const assert = require('assert');
 
-common.globalCheck = false;
+common.allowGlobals('bar', 'foo');
 
 baseFoo = 'foo'; // eslint-disable-line no-undef
 global.baseBar = 'bar';
diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js
@@ -25,9 +25,6 @@ const assert = require('assert');
 const util = require('util');
 const repl = require('repl');
 
-// This test adds global variables
-common.globalCheck = false;
-
 const putIn = new common.ArrayStream();
 repl.start('', putIn, null, true);
 
@@ -65,6 +62,7 @@ function test2() {
 };
 const val = {};
 global.url = val;
+ common.allowGlobals(val);
 assert(!gotWrite);
 putIn.run(['url']);
 assert(gotWrite);
diff --git a/test/parallel/test-repl-envvars.js b/test/parallel/test-repl-envvars.js
@@ -2,7 +2,7 @@
 
 // Flags: --expose-internals
 
-const common = require('../common');
+require('../common');
 const stream = require('stream');
 const REPL = require('internal/repl');
 const assert = require('assert');
@@ -47,9 +47,6 @@ function run(test) {
 REPL.createInternalRepl(env, opts, function(err, repl) {
 assert.ifError(err);
 
- // The REPL registers 'module' and 'require' globals
- common.allowGlobals(repl.context.module, repl.context.require);
-
 assert.strictEqual(expected.terminal, repl.terminal,
 `Expected ${inspect(expected)} with ${inspect(env)}`);
 assert.strictEqual(expected.useColors, repl.useColors,
diff --git a/test/parallel/test-repl-history-perm.js b/test/parallel/test-repl-history-perm.js
@@ -38,9 +38,6 @@ const replHistoryPath = path.join(tmpdir.path, '.node_repl_history');
 const checkResults = common.mustCall(function(err, r) {
 assert.ifError(err);
 
- // The REPL registers 'module' and 'require' globals
- common.allowGlobals(r.context.module, r.context.require);
-
 r.input.end();
 const stat = fs.statSync(replHistoryPath);
 const fileMode = stat.mode & 0o777;
diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js
@@ -216,9 +216,6 @@ function runTest(assertCleaned) {
 throw err;
 }
 
- // The REPL registers 'module' and 'require' globals
- common.allowGlobals(repl.context.module, repl.context.require);
-
 repl.once('close', () => {
 if (repl._flushing) {
 repl.once('flushHistory', onClose);
diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js
@@ -26,6 +26,8 @@ const assert = require('assert');
 const repl = require('repl');
 const util = require('util');
 
+common.allowGlobals(42);
+
 // Create a dummy stream that does nothing
 const dummy = new common.ArrayStream();
 
diff --git a/test/parallel/test-repl-use-global.js b/test/parallel/test-repl-use-global.js
@@ -18,9 +18,6 @@ const globalTest = (useGlobal, cb, output) => (err, repl) => {
 if (err)
 return cb(err);
 
- // The REPL registers 'module' and 'require' globals
- common.allowGlobals(repl.context.module, repl.context.require);
-
 let str = '';
 output.on('data', (data) => (str += data));
 global.lunch = 'tacos';
diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js
@@ -26,7 +26,6 @@ const assert = require('assert');
 const net = require('net');
 const repl = require('repl');
 
-common.globalCheck = false;
 common.crashOnUnhandledRejection();
 
 const message = 'Read, Eval, Print Loop';
@@ -41,7 +40,6 @@ global.invoke_me = function(arg) {
 return `invoked ${arg}`;
 };
 
-
 // Helpers for describing the expected output:
 const kArrow = /^ *\^+ *$/; // Arrow of ^ pointing to syntax error location
 const kSource = Symbol('kSource'); // Placeholder standing for input readback
@@ -779,6 +777,7 @@ const tcpTests = [
 
 socket.end();
 }
+ common.allowGlobals(...Object.values(global));
 })();
 
 function startTCPRepl() {
diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/test-require-deps-deprecation.js
@@ -2,8 +2,6 @@
 
 const common = require('../common');
 const assert = require('assert');
-// The v8 modules when imported leak globals. Disable global check.
-common.globalCheck = false;
 
 const deprecatedModules = [
 'node-inspect/lib/_inspect',
@@ -53,3 +51,6 @@ for (const m of deps) {
 }
 assert.notStrictEqual(path, m);
 }
+
+// The V8 modules add the WebInspector to the globals.
+common.allowGlobals(global.WebInspector);
diff --git a/test/parallel/test-timer-immediate.js b/test/parallel/test-timer-immediate.js
@@ -1,5 +1,5 @@
 'use strict';
 const common = require('../common');
-common.globalCheck = false;
 global.process = {}; // Boom!
+common.allowGlobals(global.process);
 setImmediate(common.mustCall());
diff --git a/test/parallel/test-vm-new-script-this-context.js b/test/parallel/test-vm-new-script-this-context.js
@@ -24,8 +24,6 @@ const common = require('../common');
 const assert = require('assert');
 const Script = require('vm').Script;
 
-common.globalCheck = false;
-
 // Run a string
 let script = new Script('\'passed\';');
 const result = script.runInThisContext(script);
@@ -60,3 +58,11 @@ global.f = function() { global.foo = 100; };
 script = new Script('f()');
 script.runInThisContext(script);
 assert.strictEqual(100, global.foo);
+
+common.allowGlobals(
+ global.hello,
+ global.code,
+ global.foo,
+ global.obj,
+ global.f
+);
diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js
@@ -29,8 +29,6 @@ const vm = require('vm');
 assert.strictEqual(typeof global.gc, 'function',
 'Run this test with --expose-gc');
 
-common.globalCheck = false;
-
 // Run a string
 const result = vm.runInNewContext('\'passed\';');
 assert.strictEqual(result, 'passed');
@@ -93,3 +91,10 @@ fn();
 return true;
 });
 }
+
+common.allowGlobals(
+ global.hello,
+ global.code,
+ global.foo,
+ global.obj
+);
diff --git a/test/parallel/test-vm-static-this.js b/test/parallel/test-vm-static-this.js
@@ -24,8 +24,6 @@ const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
-common.globalCheck = false;
-
 // Run a string
 const result = vm.runInThisContext('\'passed\';');
 assert.strictEqual('passed', result);
@@ -58,3 +56,10 @@ assert.strictEqual(1, global.foo);
 global.f = function() { global.foo = 100; };
 vm.runInThisContext('f()');
 assert.strictEqual(100, global.foo);
+
+common.allowGlobals(
+ global.hello,
+ global.foo,
+ global.obj,
+ global.f
+);

-Original file line number
+Diff line change
+}
 exports.leakedGlobals=leakedGlobals;
 -// Turn this off if the test should not check for global leaks.
 -exports.globalCheck=true;
+-
 process.on('exit',function(){
 -if(!exports.globalCheck)return;
 constleaked=leakedGlobals();
 if(leaked.length>0){
 assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
+}
 });
+-
 constmustCallChecks=[];
+-
 functionrunCallChecks(exitCode){
 if(exitCode!==0)return;
Original file line number	Diff line number	Diff line change
`@@ -63,11 +63,7 @@ export function leakedGlobals() {`
`63`	`63`	`}`
`64`	`64`	`}`
`65`	`65`
`66`		`-// Turn this off if the test should not check for global leaks.`
`67`		`-exportletglobalCheck=true;// eslint-disable-line`
`68`		`-`
`69`	`66`	`process.on('exit',function(){`
`70`		`-if(!globalCheck)return;`
`71`	`67`	`constleaked=leakedGlobals();`
`72`	`68`	`if(leaked.length>0){`
`73`	`69`	assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
-Original file line number
+Diff line change
 constassert=require('assert');
 -common.globalCheck=false;
 +common.allowGlobals('bar','foo');
 baseFoo='foo';// eslint-disable-line no-undef
 global.baseBar='bar';
Original file line number	Diff line number	Diff line change
`@@ -216,9 +216,6 @@ function runTest(assertCleaned) {`
`216`	`216`	`throwerr;`
`217`	`217`	`}`
`218`	`218`
`219`		`-// The REPL registers 'module' and 'require' globals`
`220`		`-common.allowGlobals(repl.context.module,repl.context.require);`
`221`		`-`
`222`	`219`	`repl.once('close',()=>{`
`223`	`220`	`if(repl._flushing){`
`224`	`221`	`repl.once('flushHistory',onClose);`