Skip to content

Commit

Permalink
test: remove common.allowGlobals
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
BridgeAR committed Apr 9, 2019
1 parent bf766c1 commit 8cddf9b
Show file tree
Hide file tree
Showing 15 changed files with 5 additions and 103 deletions.
6 changes: 0 additions & 6 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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` [<Array>] Array of Globals
* return [<Array>]

Takes `whitelist` and concats that with predefined `knownGlobals`.

### busyLoop(time)
* `time` [<number>]

Expand Down
44 changes: 0 additions & 44 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -715,7 +672,6 @@ function runWithInvalidFD(func) {
}

module.exports = {
allowGlobals,
buildType,
busyLoop,
canCreateSymLink,
Expand Down
2 changes: 0 additions & 2 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ const {
childShouldThrowAndAbort,
createZeroFilledFile,
platformTimeout,
allowGlobals,
mustCall,
mustCallAtLeast,
hasMultiLocalhost,
Expand Down Expand Up @@ -78,7 +77,6 @@ export {
childShouldThrowAndAbort,
createZeroFilledFile,
platformTimeout,
allowGlobals,
mustCall,
mustCallAtLeast,
hasMultiLocalhost,
Expand Down
5 changes: 0 additions & 5 deletions test/fixtures/leakedGlobal.js

This file was deleted.

9 changes: 0 additions & 9 deletions test/parallel/test-common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-domain-crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-fs-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 1 addition & 3 deletions test/parallel/test-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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';

Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-repl-autolibs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -63,7 +63,6 @@ function test2() {
};
const val = {};
global.url = val;
common.allowGlobals(val);
assert(!gotWrite);
putIn.run(['url']);
assert(gotWrite);
Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-repl-reset-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,6 @@ const tcpTests = [

socket.end();
}
common.allowGlobals(...Object.values(global));
})();

function startTCPRepl() {
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-timer-immediate.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
'use strict';
const common = require('../common');
global.process = {}; // Boom!
common.allowGlobals(global.process);
setImmediate(common.mustCall());
10 changes: 1 addition & 9 deletions test/parallel/test-vm-new-script-this-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
);
9 changes: 1 addition & 8 deletions test/parallel/test-vm-run-in-new-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
'use strict';
// Flags: --expose-gc

const common = require('../common');
require('../common');
const assert = require('assert');
const vm = require('vm');

Expand Down Expand Up @@ -91,10 +91,3 @@ for (const arg of [filename, { filename }]) {
return true;
});
}

common.allowGlobals(
global.hello,
global.code,
global.foo,
global.obj
);
9 changes: 1 addition & 8 deletions test/parallel/test-vm-static-this.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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
);

0 comments on commit 8cddf9b

Please sign in to comment.