test: remove common.allowGlobals

This removes the globals check that was executed in all tests so far. Instead just rely on the explicit globals test to verify that the globals did not leak.
nodejs · BridgeAR · Dec 5, 2018 · Dec 28, 2018 · Apr 9, 2019 · 8cddf9b3aaf42db271d3fbc0c32d2b0e28a5a57a
commit 8cddf9b3aaf42db271d3fbc0c32d2b0e28a5a57a
diff --git a/test/common/README.md b/test/common/README.md
@@ -33,12 +33,6 @@ The `benchmark` module is used by tests to run benchmarks.
 The `common` module is used by tests for consistency across repeated
 tasks.
 
-### allowGlobals(...whitelist)
-* `whitelist` [&lt;Array>] Array of Globals
-* return [&lt;Array>]
-
-Takes `whitelist` and concats that with predefined `knownGlobals`.
-
 ### busyLoop(time)
 * `time` [&lt;number>]
 

diff --git a/test/common/index.js b/test/common/index.js
@@ -262,49 +262,6 @@ function platformTimeout(ms) {
   return ms; // ARMv8+
 }
 
-let knownGlobals = [
-  clearImmediate,
-  clearInterval,
-  clearTimeout,
-  global,
-  setImmediate,
-  setInterval,
-  setTimeout,
-  queueMicrotask,
-];
-
-if (global.gc) {
-  knownGlobals.push(global.gc);
-}
-
-if (process.env.NODE_TEST_KNOWN_GLOBALS) {
-  const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
-  allowGlobals(...knownFromEnv);
-}
-
-function allowGlobals(...whitelist) {
-  knownGlobals = knownGlobals.concat(whitelist);
-}
-
-function leakedGlobals() {
-  const leaked = [];
-
-  for (const val in global) {
-    if (!knownGlobals.includes(global[val])) {
-      leaked.push(val);
-    }
-  }
-
-  return leaked;
-}
-
-process.on('exit', function() {
-  const leaked = leakedGlobals();
-  if (leaked.length > 0) {
-    assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
-  }
-});
-
 const mustCallChecks = [];
 
 function runCallChecks(exitCode) {
@@ -715,7 +672,6 @@ function runWithInvalidFD(func) {
 }
 
 module.exports = {
-  allowGlobals,
   buildType,
   busyLoop,
   canCreateSymLink,

diff --git a/test/common/index.mjs b/test/common/index.mjs
@@ -32,7 +32,6 @@ const {
   childShouldThrowAndAbort,
   createZeroFilledFile,
   platformTimeout,
-  allowGlobals,
   mustCall,
   mustCallAtLeast,
   hasMultiLocalhost,
@@ -78,7 +77,6 @@ export {
   childShouldThrowAndAbort,
   createZeroFilledFile,
   platformTimeout,
-  allowGlobals,
   mustCall,
   mustCallAtLeast,
   hasMultiLocalhost,

diff --git a/test/fixtures/leakedGlobal.js b/test/fixtures/leakedGlobal.js
diff --git a/test/parallel/test-common.js b/test/parallel/test-common.js
@@ -26,15 +26,6 @@ const fixtures = require('../common/fixtures');
 const assert = require('assert');
 const { execFile } = require('child_process');
 
-// Test for leaked global detection
-{
-  const p = fixtures.path('leakedGlobal.js');
-  execFile(process.execPath, [p], common.mustCall((err, stdout, stderr) => {
-    assert.notStrictEqual(err.code, 0);
-    assert.ok(/\bAssertionError\b.*\bUnexpected global\b.*\bgc\b/.test(stderr));
-  }));
-}
-
 // common.mustCall() tests
 assert.throws(function() {
   common.mustCall(function() {}, 'foo');

diff --git a/test/parallel/test-domain-crypto.js b/test/parallel/test-domain-crypto.js
@@ -29,7 +29,6 @@ if (!common.hasCrypto)
 const crypto = require('crypto');
 
 // Pollution of global is intentional as part of test.
-common.allowGlobals(require('domain'));
 // See https://github.com/nodejs/node/commit/d1eff9ab
 global.domain = require('domain');
 

diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js
@@ -36,8 +36,6 @@ const expected = 'ümlaut.';
 const constants = fs.constants;
 
 /* eslint-disable no-undef */
-common.allowGlobals(externalizeString, isOneByteString, x);
-
 {
   const expected = 'ümlaut eins';  // Must be a unique string.
   externalizeString(expected);

diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js
@@ -23,7 +23,7 @@
 // treated as a global without being declared with `var`/`let`/`const`.
 
 /* eslint-disable strict */
-const common = require('../common');
+require('../common');
 const fixtures = require('../common/fixtures');
 
 const assert = require('assert');
@@ -54,8 +54,6 @@ builtinModules.forEach((moduleName) => {
   assert.deepStrictEqual(new Set(Object.keys(global)), new Set(expected));
 }
 
-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
@@ -20,7 +20,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 'use strict';
-const common = require('../common');
+require('../common');
 const ArrayStream = require('../common/arraystream');
 const assert = require('assert');
 const util = require('util');
@@ -63,7 +63,6 @@ function test2() {
   };
   const val = {};
   global.url = val;
-  common.allowGlobals(val);
   assert(!gotWrite);
   putIn.run(['url']);
   assert(gotWrite);

diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js
@@ -26,8 +26,6 @@ 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 ArrayStream();
 

diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js
@@ -770,7 +770,6 @@ const tcpTests = [
 
     socket.end();
   }
-  common.allowGlobals(...Object.values(global));
 })();
 
 function startTCPRepl() {

diff --git a/test/parallel/test-timer-immediate.js b/test/parallel/test-timer-immediate.js
@@ -1,5 +1,4 @@
 'use strict';
 const common = require('../common');
 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
@@ -20,7 +20,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 'use strict';
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 const Script = require('vm').Script;
 
@@ -58,11 +58,3 @@ global.f = function() { global.foo = 100; };
 script = new Script('f()');
 script.runInThisContext(script);
 assert.strictEqual(global.foo, 100);
-
-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
@@ -22,7 +22,7 @@
 'use strict';
 // Flags: --expose-gc
 
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
@@ -91,10 +91,3 @@ for (const arg of [filename, { filename }]) {
     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
@@ -20,7 +20,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 /* eslint-disable strict */
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
@@ -56,10 +56,3 @@ assert.strictEqual(global.foo, 1);
 global.f = function() { global.foo = 100; };
 vm.runInThisContext('f()');
 assert.strictEqual(global.foo, 100);
-
-common.allowGlobals(
-  global.hello,
-  global.foo,
-  global.obj,
-  global.f
-);
-Original file line number
+Diff line change
@@ Expand Up / @@ -770,7 +770,6 @@ const tcpTests = [ @@
         socket.end();
       }
-      common.allowGlobals(...Object.values(global));
     })();
     function startTCPRepl() {
@@ Expand Down @@