Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: compute list of expected globals from ESLint config file #42056

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
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