Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ _UpgradeReport_Files/
tools/*/*.i
tools/*/*.i.tmp

test/common/knownGlobals.json

# === Rules for release artifacts ===
/*.tar.*
/*.pkg
Expand Down
11 changes: 8 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ testclean:
# Next one is legacy remove this at some point
$(RM) -r test/tmp*
$(RM) -r test/.tmp*
$(RM) -d test/common/knownGlobals.json

.PHONY: distclean
.NOTPARALLEL: distclean
Expand Down Expand Up @@ -280,8 +281,11 @@ v8:
export PATH="$(NO_BIN_OVERRIDE_PATH)" && \
tools/make-v8.sh $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)

test/common/knownGlobals.json: test/common/parseEslintConfigForKnownGlobals.js lib/.eslintrc.yaml
$(NODE) $< > $@

.PHONY: jstest
jstest: build-addons build-js-native-api-tests build-node-api-tests ## Runs addon tests and JS tests
jstest: build-addons build-js-native-api-tests build-node-api-tests test/common/knownGlobals.json ## Runs addon tests and JS tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is going to break jstest (and targets such as test-only) for builds from the source tar ball as we strip out anything related to linting (e.g. lib/.eslintrc.yaml) from those: #5618

node/Makefile

Line 1167 in aa97c9d

find $(TARNAME)/ -name ".eslint*" -maxdepth 2 | xargs $(RM)

$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \
$(TEST_CI_ARGS) \
--skip-tests=$(CI_SKIP_TESTS) \
Expand Down Expand Up @@ -606,7 +610,7 @@ test-doc: doc-only lint-md ## Builds, lints, and verifies the docs.
fi

.PHONY: test-doc-ci
test-doc-ci: doc-only
test-doc-ci: doc-only test/common/knownGlobals.json
$(PYTHON) tools/test.py --shell $(NODE) $(TEST_CI_ARGS) $(PARALLEL_ARGS) doctool

.PHONY: test-known-issues
Expand Down Expand Up @@ -1136,7 +1140,7 @@ pkg-upload: pkg
scp -p $(TARNAME).pkg $(STAGINGSERVER):nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/$(TARNAME).pkg
ssh $(STAGINGSERVER) "touch nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/$(TARNAME).pkg.done"

$(TARBALL): release-only doc-only
$(TARBALL): release-only doc-only test/common/knownGlobals.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This target will also probably need to copy in the generated test/common/knownGlobals.json as the tarball is prepared in $(TARNAME)/ from a fresh git checkout-index.

git checkout-index -a -f --prefix=$(TARNAME)/
mkdir -p $(TARNAME)/doc/api
cp doc/node.1 $(TARNAME)/doc/node.1
Expand All @@ -1155,6 +1159,7 @@ $(TARBALL): release-only doc-only
$(RM) -r $(TARNAME)/deps/v8/tools/run-tests.py
$(RM) -r $(TARNAME)/doc/images # too big
$(RM) -r $(TARNAME)/test*.tap
$(RM) -r $(TARNAME)/test/common/parseEslintConfigForKnownGlobals.js
$(RM) -r $(TARNAME)/tools/cpplint.py
$(RM) -r $(TARNAME)/tools/eslint-rules
$(RM) -r $(TARNAME)/tools/license-builder.sh
Expand Down
73 changes: 12 additions & 61 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@ const bits = ['arm64', 'mips', 'mipsel', 'ppc64', 'riscv64', 's390x', 'x64']
.includes(process.arch) ? 64 : 32;
const hasIntl = !!process.config.variables.v8_enable_i18n_support;

const {
atob,
btoa
} = require('buffer');

// Some tests assume a umask of 0o022 so set that up front. Tests that need a
// different umask will set it themselves.
//
Expand Down Expand Up @@ -257,61 +252,16 @@ function platformTimeout(ms) {
return ms; // ARMv8+
}

let knownGlobals = [
atob,
btoa,
clearImmediate,
clearInterval,
clearTimeout,
global,
setImmediate,
setInterval,
setTimeout,
queueMicrotask,
];

// TODO(@jasnell): This check can be temporary. AbortController is
// not currently supported in either Node.js 12 or 10, making it
// difficult to run tests comparatively on those versions. Once
// all supported versions have AbortController as a global, this
// check can be removed and AbortController can be added to the
// knownGlobals list above.
if (global.AbortController)
knownGlobals.push(global.AbortController);

if (global.gc) {
knownGlobals.push(global.gc);
}

if (global.performance) {
knownGlobals.push(global.performance);
}
if (global.PerformanceMark) {
knownGlobals.push(global.PerformanceMark);
}
if (global.PerformanceMeasure) {
knownGlobals.push(global.PerformanceMeasure);
}

// TODO(@ethan-arrowood): Similar to previous checks, this can be temporary
// until v16.x is EOL. Once all supported versions have structuredClone we
// can add this to the list above instead.
if (global.structuredClone) {
knownGlobals.push(global.structuredClone);
}

if (global.fetch) {
knownGlobals.push(fetch);
}
if (hasCrypto && global.crypto) {
knownGlobals.push(global.crypto);
knownGlobals.push(global.Crypto);
knownGlobals.push(global.CryptoKey);
knownGlobals.push(global.SubtleCrypto);
let knownGlobals;
try {
knownGlobals = require('./knownGlobals.json');
} catch (err) {
console.info('You may need to run `make test/common/knownGlobals.json`.');
throw err;
}

function allowGlobals(...allowlist) {
knownGlobals = knownGlobals.concat(allowlist);
knownGlobals.push(...allowlist);
}

if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
Expand All @@ -323,9 +273,10 @@ if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
function leakedGlobals() {
const leaked = [];

for (const val in global) {
if (!knownGlobals.includes(global[val])) {
leaked.push(val);
const globals = Object.getOwnPropertyDescriptors(global);
for (const val in globals) {
if (globals[val].configurable && !knownGlobals.includes(val) && !knownGlobals.includes(global[val])) {
leaked.push(val.toString());
}
}

Expand All @@ -335,7 +286,7 @@ if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
process.on('exit', function() {
const leaked = leakedGlobals();
if (leaked.length > 0) {
assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}. Add it to lib/.eslint.yaml or call common.allowGlobals().`);
}
});
}
Expand Down
45 changes: 45 additions & 0 deletions test/common/parseEslintConfigForKnownGlobals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/usr/bin/env node
'use strict';

const fs = require('fs');
const path = require('path');
const readline = require('readline');

const searchLines = [
' no-restricted-globals:',
' node-core/prefer-primordials:',
];

const restrictedGlobalLine = /^\s{4}- name:\s?([^#\s]+)/;
const closingLine = /^\s{0,3}[^#\s]/;

const jsonFile = fs.openSync(path.join(__dirname, 'knownGlobals.json'), 'w');

fs.writeSync(jsonFile, '["process"');

const eslintConfig = readline.createInterface({
input: fs.createReadStream(
path.join(__dirname, '..', '..', 'lib', '.eslintrc.yaml')
),
crlfDelay: Infinity,
});

let isReadingGlobals = false;
eslintConfig.on('line', (line) => {
if (isReadingGlobals) {
const match = restrictedGlobalLine.exec(line);
if (match != null) {
fs.writeSync(jsonFile, ',' + JSON.stringify(match[1]));
} else if (closingLine.test(line)) {
isReadingGlobals = false;
}
} else if (line === searchLines[0]) {
searchLines.shift();
isReadingGlobals = true;
}
});

eslintConfig.once('close', () => {
fs.writeSync(jsonFile, ']');
fs.closeSync(jsonFile);
});
2 changes: 2 additions & 0 deletions test/parallel/test-repl-autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ common.skipIfDumbTerminal();
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add common.allowReplGlobals() or similar to avoid repetition?


process.throwDeprecation = true;

const defaultHistoryPath = path.join(tmpdir.path, '.node_repl_history');
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-autolibs.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const assert = require('assert');
const util = require('util');
const repl = require('repl');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

const putIn = new ArrayStream();
repl.start('', putIn, null, true);

Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-repl-envvars.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

// Flags: --expose-internals

require('../common');
const common = require('../common');
const stream = require('stream');
const REPL = require('internal/repl');
const assert = require('assert');
const inspect = require('util').inspect;
const { REPL_MODE_SLOPPY, REPL_MODE_STRICT } = require('repl');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

const tests = [
{
env: {},
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-history-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ tmpdir.refresh();
process.throwDeprecation = true;
process.on('warning', common.mustNotCall());

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

const defaultHistoryPath = path.join(tmpdir.path, '.node_repl_history');

// Create an input stream specialized for testing an array of actions
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-history-perm.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const Duplex = require('stream').Duplex;
// Invoking the REPL should create a repl history file at the specified path
// and mode 600.

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

const stream = new Duplex();
stream.pause = stream.resume = () => {};
// ends immediately
Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-repl-let-process.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
'use strict';
require('../common');
const common = require('../common');
const ArrayStream = require('../common/arraystream');
const repl = require('repl');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

// Regression test for https://github.com/nodejs/node/issues/6802
const input = new ArrayStream();
repl.start({ input, output: process.stdout, useGlobal: true });
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const assert = require('assert');
const repl = require('repl');
const cp = require('child_process');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

assert.strictEqual(repl.repl, undefined);
repl._builtinLibs; // eslint-disable-line no-unused-expressions

Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-persistent-history.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ common.skipIfDumbTerminal();
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

// Mock os.homedir()
os.homedir = function() {
return tmpdir.path;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-repl-reset-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const assert = require('assert');
const repl = require('repl');
const util = require('util');

common.allowGlobals(42);
common.allowGlobals(42, 'require', '_', '_error', ...require('module').builtinModules);

// Create a dummy stream that does nothing
const dummy = new ArrayStream();
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-reverse-search.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ common.allowGlobals('aaaa');
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

const defaultHistoryPath = path.join(tmpdir.path, '.node_repl_history');

// Create an input stream specialized for testing an array of actions
Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-repl-underscore.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
'use strict';

require('../common');
const common = require('../common');
const assert = require('assert');
const repl = require('repl');
const stream = require('stream');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

testSloppyMode();
testStrictMode();
testResetContext();
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-repl-use-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const stream = require('stream');
const repl = require('internal/repl');
const assert = require('assert');

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);

// Array of [useGlobal, expectedResult] pairs
const globalTestCases = [
[false, 'undefined'],
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const moduleFilename = fixtures.path('a');
global.invoke_me = function(arg) {
return `invoked ${arg}`;
};
common.allowGlobals('invoke_me', 'require', 'a', '_', '_error', 'message', ...require('module').builtinModules);

// Helpers for describing the expected output:
const kArrow = /^ *\^+ *$/; // Arrow of ^ pointing to syntax error location
Expand Down
1 change: 1 addition & 0 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ assert.throws(

assert.deepStrictEqual(children, {
'common/index.js': {
'common/knownGlobals.json': {},
'common/tmpdir.js': {}
},
'fixtures/not-main-module.js': {},
Expand Down
2 changes: 2 additions & 0 deletions tools/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,8 @@ def Main():
if has_crypto.stdout.rstrip() == 'undefined':
context.node_has_crypto = False

Execute([vm, join(workspace, "tools", "common", "parseEslintConfigForKnownGlobals.js")], context)

if options.cat:
visited = set()
for test in unclassified_tests:
Expand Down