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 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`		`-export let globalCheck = true; // eslint-disable-line`
`68`		`-`
`69`	`66`	`process.on('exit', function() {`
`70`		`- if (!globalCheck) return;`
`71`	`67`	`const leaked = leakedGlobals();`
`72`	`68`	`if (leaked.length > 0) {`
`73`	`69`	assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
Original file line number	Diff line number	Diff line change
`@@ -216,9 +216,6 @@ function runTest(assertCleaned) {`
`216`	`216`	`throw err;`
`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);`