From 22f46e7766815aa500c38c41e28b5c3824879a4f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 May 2018 16:16:33 +0200 Subject: [PATCH] test: remove common.globalCheck 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: https://github.com/nodejs/node/pull/20717 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- test/common/README.md | 5 ----- test/common/index.js | 6 ------ test/common/index.mjs | 4 ---- test/parallel/test-domain-crypto.js | 2 +- test/parallel/test-global.js | 2 +- test/parallel/test-repl-autolibs.js | 4 +--- test/parallel/test-repl-envvars.js | 5 +---- test/parallel/test-repl-history-perm.js | 3 --- test/parallel/test-repl-persistent-history.js | 3 --- test/parallel/test-repl-reset-event.js | 2 ++ test/parallel/test-repl-use-global.js | 3 --- test/parallel/test-repl.js | 3 +-- test/parallel/test-require-deps-deprecation.js | 5 +++-- test/parallel/test-timer-immediate.js | 2 +- test/parallel/test-vm-new-script-this-context.js | 10 ++++++++-- test/parallel/test-vm-run-in-new-context.js | 9 +++++++-- test/parallel/test-vm-static-this.js | 9 +++++++-- 17 files changed, 33 insertions(+), 44 deletions(-) diff --git a/test/common/README.md b/test/common/README.md index 08d7bbb762fbb4..111ce45b00360c 100644 --- 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 -* [<boolean>] - -Set to `false` if the test should not check for global leaks. - ### hasCrypto * [<boolean>] diff --git a/test/common/index.js b/test/common/index.js index 743bf15fb8301d..85d11ec093414c 100644 --- 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 index dd4ee2146e0720..b7ac556bca52c3 100644 --- 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 index 3890a7b4641ade..26ee6b888efb2d 100644 --- 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 index 46db72eeceb586..b139a6287488d2 100644 --- 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 index 52234deb5e732e..024dd971bf44e1 100644 --- 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 index d29e7b3574c1f2..d6a45a664fa3b7 100644 --- 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 index b125fa551dc858..03ce1435d9ba4e 100644 --- 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 index 4d0330272ab3c6..bb10085eccfcf6 100644 --- 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 index 4b3a04aeeb88c5..96d1d199af34c9 100644 --- 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 index c76505272b2682..947adc7f255035 100644 --- 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 index b9c92c3cd63bc0..9aaf366c6807f0 100644 --- 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 index 24a2e86e6a42ee..80bf66d7f6b74f 100644 --- 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 index 385fa4baca4ee5..b0f52db1b713d3 100644 --- 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 index 6da47e67249c2b..2c0c21690e1821 100644 --- 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 index 1edb061ea6a871..e844cd6816bba3 100644 --- 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 index 5306e31dc0e4a7..d4530604dc06f0 100644 --- 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 +);