Skip to content

Commit

Permalink
test: do not export common.leakedGlobals()
Browse files Browse the repository at this point in the history
common.leakedGlobals() was exposed only to test its logic. The logic can
instead be tested by running a fixture file that leaks a global and
seeing if `common` causes an AssertionError on exit. This way, the
entire functionality of leak detection is tested rather than just the
leakedGlobals() function. It also reduces API surface area for the
common monolith by one function.

PR-URL: #22965
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Trott authored and targos committed Sep 23, 2018
1 parent d3bc862 commit 43e3cf9
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 12 deletions.
5 changes: 0 additions & 5 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,6 @@ Platform check for Windows.

Platform check for Windows 32-bit on Windows 64-bit.

### leakedGlobals()
* return [&lt;Array>]

Indicates whether any globals are not on the `knownGlobals` list.

### localhostIPv4
* [&lt;string>]

Expand Down
1 change: 0 additions & 1 deletion test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,6 @@ module.exports = {
isSunOS,
isWindows,
isWOW64,
leakedGlobals,
localIPv6Hosts,
mustCall,
mustCallAsync,
Expand Down
2 changes: 0 additions & 2 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ const {
ddCommand,
platformTimeout,
allowGlobals,
leakedGlobals,
mustCall,
mustCallAtLeast,
mustCallAsync,
Expand Down Expand Up @@ -75,7 +74,6 @@ export {
ddCommand,
platformTimeout,
allowGlobals,
leakedGlobals,
mustCall,
mustCallAtLeast,
mustCallAsync,
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/leakedGlobal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

require('../common');

global.gc = 42; // intentionally leak a global
11 changes: 7 additions & 4 deletions test/parallel/test-common.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ const assert = require('assert');
const { execFile } = require('child_process');

// test for leaked global detection
global.gc = 42; // Not a valid global unless --expose_gc is set.
assert.deepStrictEqual(common.leakedGlobals(), ['gc']);
delete global.gc;

{
const p = fixtures.path('leakedGlobal.js');
execFile(process.argv[0], [p], common.mustCall((ex, stdout, stderr) => {
assert.notStrictEqual(ex.code, 0);
assert.ok(/\bAssertionError\b.*\bUnexpected global\b.*\bgc\b/.test(stderr));
}));
}

// common.mustCall() tests
assert.throws(function() {
Expand Down

0 comments on commit 43e3cf9

Please sign in to comment.