test: remove common.allowGlobals

[BridgeAR]

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.

We have dedicated tests for the globals (/test/parallel/test-global.js) which load all modules up front to make sure we definitely get all code which might add globals.

This test is very old and comes from times where we neither had code reviews nor good tests. Now we have both and I don't think we should run this code on each test file anymore.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2094/pipeline

[Trott]

We have dedicated tests for the globals (/test/parallel/test-global.js) which load all modules up front to make sure we definitely get all code which might add globals.

It loads all modules? I don't see that in that test. How do we make sure (for example) that child_process doesn't leak a global when it is loaded?

[Trott]

I'm interested that you think this should be removed, as I've kind of felt it's ideally the only thing common should do by default. 😀(EDIT: As in, you can get the other functionality of common but you have to ask for it. This is the thing that you get no matter what.)

[BridgeAR]

@Trott

It loads all modules? I don't see that in that test.

It does not load the internal modules so far (which I thought is not really required due to implicitly loading them when loading the regular modules but I am happy to load them as well by adding the --expose-internals flag).

How do we make sure (for example) that child_process doesn't leak a global when it is loaded?

node/test/parallel/test-global.js

Lines 30 to 41 in c992639

	const { builtinModules } = require('module');
	// Load all modules to actually cover most code parts.
	builtinModules.forEach((moduleName) => {
	if (!moduleName.includes('/')) {
	try {
	// This could throw for e.g., crypto if the binary is not compiled
	// accordingly.
	require(moduleName);
	} catch {}
	}
	});

Here I made sure to load all regular modules up front. Therefore the child_process is also loaded before verifying the globals right afterwards.

node/test/parallel/test-global.js

Lines 44 to 63 in c992639

	const expected = [
	'global',
	'clearImmediate',
	'clearInterval',
	'clearTimeout',
	'setImmediate',
	'setInterval',
	'setTimeout'
	];
	if (global.DTRACE_HTTP_SERVER_RESPONSE) {
	expected.unshift(
	'DTRACE_HTTP_SERVER_RESPONSE',
	'DTRACE_HTTP_SERVER_REQUEST',
	'DTRACE_HTTP_CLIENT_RESPONSE',
	'DTRACE_HTTP_CLIENT_REQUEST',
	'DTRACE_NET_STREAM_END',
	'DTRACE_NET_SERVER_CONNECTION'
	);
	}
	assert.deepStrictEqual(new Set(Object.keys(global)), new Set(expected));

Theoretically, the execution of some code could still add a global somewhere but I feel very safe with this test (and in the very unlikely case that not, we would still have our code reviews).

[lpinca]

What is the advantage in removing it? It doesn't seem to be a maintenance burden and to have a significant overhead when running tests so I think that what we lose is more than what we gain if we remove it.

[cjihrig]

Theoretically, the execution of some code could still add a global somewhere but I feel very safe with this test (and in the very unlikely case that not, we would still have our code reviews).

I don't share this sentiment, and am -1 to removing this.

[BridgeAR]

@cjihrig would a lint rule be sufficient on top to detect attaching things to global?

@lpinca the only thing I know is that we had to change this code multiple times (and we also made mistakes while working on it) and we have to make sure that our tests work properly with this (which is not that much work since we normally do not manipulate globals in tests either).

---

Are we really running into any danger adding globals to the code base without this check? For me this is just a bunch of code in the common part that does not seem to have a real benefit.

If we keep this check, we should remove the part in the /test/parallel/test-global.js test again as that's just doing the same check again.

I'll keep this open a bit longer but if I am really alone with my view, I'll close it depending on following comments / in a few days.

[Trott]

Here I made sure to load all regular modules up front.

How did I miss that? Yeah, that seems fine. Sorry to make you spell it out in such painful detail.

I can imagine code in a module that would pollute the global namespace that wouldn't get caught by the one test.

exports.foo = () => { global.fhqwhgads = 'come on'; }

That won't pollute the global namespace by loading the module, but it will by executing .foo(). So that's an argument for checking all tests.

I guess the questions are: Is that scenario above all that likely? And is the cost of checking high?

I'd say it's low risk but also low cost to check.

[BridgeAR]

I just added a custom eslint rule to verify that no global is being accessed at all anywhere under lib. To me this seems a much cleaner solution than checking in each test if the globals were modified. The check is very simple and marks any global as wrong.

I removed access to it from all files that did not require it and the rule is only ignored in a single file: the bootstrap file where we actually go ahead and add such entries regularly.

Now any code that modifies them would also have to deactivate the eslint rule to do so.

Opinions?

[Trott]

Opinions?

Seems OK, but there are things that ESLint can't catch that our current setup would, like polluting global through eval() or something like this:

function setup(obj, key, val) {
   obj[key] = val;
}

setup(global, 'foo', 'bar');

As a general guideline, linting is for style and testing is for correctness, so this is properly the purview of tests, not linting.

[BridgeAR]

@Trott your example would be caught because is using global as variable and the rule I wrote is there to prohibit the usage of it in general. It won't detect eval but that's normally not used in our code in general.

As a general guideline, linting is for style and testing is for correctness

I don't fully agree: static analysis is very strong and we catch many errors early on which our tests might not always detect. Other languages do the same: they just don't compile in cases where eslint finds correctness issues :D

@cjihrig @lpinca PTAL

[BridgeAR]

readable-streams pulls in all our tests but only from time to time. This breaks CITGM for readable-streams right now because we added a new global that is unknown in the whitelist.

Even with this PR readable-streams would still fail on CITGM but it would only show one test instead of almost every single test as failed.

I updated the custom eslint rule to detect global usage and to prohibit that in lib (with the exception of bootstrap because we use that to set up the globals).

@nodejs/testing is this something we want to do or should I close this PR?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22167/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22176/

[BridgeAR]

I am frequently running into issues due to the current test. I often run tests with older Node.js versions to compare some output and that is not possible since tests fail due to added globals in newer Node.js versions.

I would really love to make some progress here and get some reviews or at least further opinions.

events.js:173
      throw er; // Unhandled 'error' event
      ^
AssertionError [ERR_ASSERTION]: Unexpected global(s) found: process, Buffer
    at process.<anonymous> (/home/ruben/repos/node/node/test/common/index.js:304:12)
    at process.emit (events.js:197:13)

[joyeecheung]

I often run tests with older Node.js versions to compare some output and that is not possible since tests fail due to added globals in newer Node.js versions.

I don't think tests are supposed to work on older versions of Node.js anyway? If this is necessary, I think a better way to solve this is to add a special environment variable that skips the global check, instead of just removing the check unconditionally.

[Trott]

I often run tests with older Node.js versions to compare some output and that is not possible since tests fail due to added globals in newer Node.js versions.

I do the same and I'm surprised that the globals thing is a noticeable problem. I'm guessing the older versions of Node.js aren't that much older? Running with earlier than 10.x fails because common uses worker_threads with no flag. Running with 8.x fails due to a syntax error!

So I usually end up commenting out the common import entirely and polyfilling other stuff that uses methods and properties in common. It's one of the reasons I'm resistant to trying to DRY out the tests by putting more stuff in common. The more stuff there is in common, the harder it is to do that.

I remain opposed to removing the global leak check. I agree with @joyeecheung that an environment variable disabling it (or a CLI flag or even a common.disableGlobalLeakCheck()) would be a better approach than removing it from all or nearly all tests.

[BridgeAR]

@joyeecheung

I don't think tests are supposed to work on older versions of Node.js anyway

I normally use the latest current and mostly the tests still work in older Node.js versions, since we do not break that much. That might not apply to all code but at least to the things I often work with.

@Trott

I'm guessing the older versions of Node.js aren't that much older

Correct, while I some times also use older versions, I mainly use the latest current to verify that a test change would have the identical behavior in the older version.

It's one of the reasons I'm resistant to trying to DRY out the tests by putting more stuff in common. The more stuff there is in common, the harder it is to do that.

I agree. I personally would like common to be used less. Ideally most tests would just work without most common functionality. That way a lot of tests would work significantly better across multiple Node.js versions.

I remain opposed to removing the global leak check.

It seems like most people still want to keep it. Thanks for your feedback.