A bit of a tangent but: Not sure if this will get a 👍 or a 👎 from the maintainers, but either way, if the common.js file is of interest to you, one thing that we definitely need is improved documentation of its properties in test/README.md.

For example, current documentation for allowGlobals() says:

Takes whitelist and concats that with predefined knownGlobals.

That's a correct description of the algorithm, but it doesn't tell you what I might use the function for. (It appends whitelist to the list of known global variables so that variables listed in whitelist are not flagged as unintentionally leaked globals by common.js. It's a way of saying, "I know I'm leaking these global variables, don't warn me about them.")

(No disrespect to @paulgrock who put those initial descriptions there in the first place. Before that, we had nothing. Now, we have a starting point!)

commit message follows commit guidelines

It doesn't though, please format the commit message according to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

Atm, it is one-line and says:

test: swaps var for let/const throughout

@ChALkeR I enhanced the commit message with the explanation from this PR. Thought the commit was self explanatory but have read the commit guidelines more thoroughly. Thanks!

Tiny nit on the commit message (which someone can fix when landing if you don't get around to it): swaps -> swap on the first line. Guidelines say the verb should be "imperative mode" (or at least, that's what I recall) so swap (active command-like) rather than swaps (descriptive).

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

@Trott CI failures seem to be due to pull issues with Jenkins, not code

Jenkins was having issues. Let's try again.

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

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

Single CI failure seems unrelated, but since common.js is central to the tests, let's run again.

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

@Trott still seems like maybe some technical issues with CI

NEVER TOO MANY CI RUNS!

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

CAN'T GET ENOUGH CI!

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

(CI is ✅ even if the widget says otherwise.)

@homosaur I was reading your commit message

The exception is line 42 in the rimrafSync function. Both let and const are causing tons of test failures in that instance.

This seems to work

diff --git a/test/common.js b/test/common.js
index e231547fc5..d138310aa6 100644
--- a/test/common.js
+++ b/test/common.js
@@ -41,8 +41,9 @@ exports.rootDir = exports.isWindows ? 'c:\\' : '/';
 exports.buildType = process.config.target_defaults.default_configuration;
 
 function rimrafSync(p) {
+  let st;
   try {
-    var st = fs.lstatSync(p);
+    st = fs.lstatSync(p);
   } catch (e) {
     if (e.code === 'ENOENT')
       return;

are you ok with swapping that last var?

Sure, @lpinca, I'm glad you found a way around that. I'm curious what causes the variable to need to be instantiated outside the try/catch block but I think it's a way to remove that last bit. Thanks for taking a look at that line.

I'm guessing that try/catch is considered a scoping block for let, which is why it doesn't then understand the st call in the next try/catch block. That makes sense logically.

@homosaur that's because let is block scoped while var isn't.

@homosaur exactly :)

@lpinca Changed block to your suggestion, test suite does pass. Thanks!

@homosaur thanks, can I ask one last thing?

Would you mind updating the commit message to reflect the changes?
Something like this should work:

test: swap var for let/const in common.js module

* swap var for let/const throughout the common.js module
* change variable name from openssl_cmd to opensslCmd

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

Linter error was probably the one that was in master for a few hours and not in this PR. Running linter again: https://ci.nodejs.org/job/node-test-linter/6015/

Landed in ecd11b6

This has landed cleanly in v6.x without any problems. Please feel free to manually backport to v4.x-staging