test: swaps var for let/const throughout common module #10177

homosaur · 2016-12-08T05:52:45Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

Swaps var for let/const throughout the common.js module.

Also changes a rogue snake case var to camel case.

Trott · 2016-12-08T06:32:54Z

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!)

cjihrig

LGTM if the CI passes.

ChALkeR

LGTM if the CI check passes.

ChALkeR · 2016-12-08T17:05:09Z

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

homosaur · 2016-12-08T19:01:26Z

@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!

Trott · 2016-12-08T19:29:29Z

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).

Trott · 2016-12-08T19:30:06Z

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

homosaur · 2016-12-09T03:41:43Z

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

Trott · 2016-12-09T05:45:10Z

Jenkins was having issues. Let's try again.

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

thefourtheye · 2016-12-09T17:20:36Z

test/common.js

@@ -165,7 +165,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {

  if (exports.isWindows) opensslCli += '.exe';

-  var openssl_cmd = child_process.spawnSync(opensslCli, ['version']);
+  const openssl_cmd = child_process.spawnSync(opensslCli, ['version']);


Now that you are changing this line, can you change the variable name to use camelCase?

@thefourtheye sure!

Thanks @homosaur :-)

Trott · 2016-12-09T23:18:47Z

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

Trott · 2016-12-10T05:29:35Z

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/

homosaur · 2016-12-13T22:27:39Z

@Trott still seems like maybe some technical issues with CI

Trott · 2016-12-14T03:36:00Z

NEVER TOO MANY CI RUNS!

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

Trott · 2016-12-14T04:37:25Z

CAN'T GET ENOUGH CI!

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

Trott · 2016-12-14T22:42:00Z

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

lpinca · 2016-12-16T13:23:44Z

@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?

homosaur · 2016-12-16T15:42:24Z

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.

homosaur · 2016-12-16T15:46:21Z

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.

lpinca · 2016-12-16T15:46:29Z

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

lpinca · 2016-12-16T15:46:49Z

@homosaur exactly :)

homosaur · 2016-12-16T15:51:36Z

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

lpinca · 2016-12-16T15:56:30Z

@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

lpinca · 2016-12-16T16:13:01Z

test/common.js

-    // openssl command cannot be executed
+  const opensslCmd = child_process.spawnSync(opensslCli, ['version']);
+  if (opensslCmd.status !== 0 || opensslCmd.error !== undefined) {
+    // wopenssl command cannot be executed


Nit: typo on "wopenssl" ?

Swap var for let/const throughout the common.js module. Change a snake case variable to camel case starting on line 168.

jasnell · 2016-12-23T05:50:45Z

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

Trott · 2016-12-23T19:56:28Z

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/

Swap var for let/const throughout the common.js module. Change a snake case variable to camel case starting on line 168. PR-URL: #10177 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-12-27T22:29:03Z

Landed in ecd11b6

Swap var for let/const throughout the common.js module. Change a snake case variable to camel case starting on line 168. PR-URL: #10177 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-01-23T00:55:17Z

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

Swap var for let/const throughout the common.js module. Change a snake case variable to camel case starting on line 168. PR-URL: #10177 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 8, 2016

cjihrig approved these changes Dec 8, 2016

View reviewed changes

ChALkeR approved these changes Dec 8, 2016

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 8, 2016

Fishrock123 removed the dont-land-on-v7.x label Dec 8, 2016

nodejs-github-bot added the dont-land-on-v7.x label Dec 8, 2016

Fishrock123 removed the dont-land-on-v7.x label Dec 8, 2016

thefourtheye approved these changes Dec 9, 2016

View reviewed changes

lpinca approved these changes Dec 14, 2016

View reviewed changes

cjihrig approved these changes Dec 16, 2016

View reviewed changes

lpinca reviewed Dec 16, 2016

View reviewed changes

test: swap var for let/const throughout

81c4be7

Swap var for let/const throughout the common.js module. Change a snake case variable to camel case starting on line 168.

jasnell self-assigned this Dec 23, 2016

jasnell approved these changes Dec 27, 2016

View reviewed changes

jasnell closed this Dec 27, 2016

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added dont-land-on-v4.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: swaps var for let/const throughout common module #10177

test: swaps var for let/const throughout common module #10177

homosaur commented Dec 8, 2016 •

edited

Loading

Trott commented Dec 8, 2016 •

edited

Loading

cjihrig left a comment

ChALkeR left a comment •

edited

Loading

ChALkeR commented Dec 8, 2016

homosaur commented Dec 8, 2016

Trott commented Dec 8, 2016

Trott commented Dec 8, 2016

homosaur commented Dec 9, 2016

Trott commented Dec 9, 2016

thefourtheye Dec 9, 2016

homosaur Dec 9, 2016

thefourtheye Dec 10, 2016

Trott commented Dec 9, 2016

Trott commented Dec 10, 2016

homosaur commented Dec 13, 2016

Trott commented Dec 14, 2016

Trott commented Dec 14, 2016

Trott commented Dec 14, 2016

lpinca commented Dec 16, 2016

homosaur commented Dec 16, 2016

homosaur commented Dec 16, 2016

lpinca commented Dec 16, 2016

lpinca commented Dec 16, 2016

homosaur commented Dec 16, 2016

lpinca commented Dec 16, 2016

lpinca Dec 16, 2016

jasnell commented Dec 23, 2016

Trott commented Dec 23, 2016

jasnell commented Dec 27, 2016

MylesBorins commented Jan 23, 2017

test: swaps var for let/const throughout common module #10177

test: swaps var for let/const throughout common module #10177

Conversation

homosaur commented Dec 8, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Trott commented Dec 8, 2016 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

ChALkeR left a comment • edited Loading

Choose a reason for hiding this comment

ChALkeR commented Dec 8, 2016

homosaur commented Dec 8, 2016

Trott commented Dec 8, 2016

Trott commented Dec 8, 2016

homosaur commented Dec 9, 2016

Trott commented Dec 9, 2016

thefourtheye Dec 9, 2016

Choose a reason for hiding this comment

homosaur Dec 9, 2016

Choose a reason for hiding this comment

thefourtheye Dec 10, 2016

Choose a reason for hiding this comment

Trott commented Dec 9, 2016

Trott commented Dec 10, 2016

homosaur commented Dec 13, 2016

Trott commented Dec 14, 2016

Trott commented Dec 14, 2016

Trott commented Dec 14, 2016

lpinca commented Dec 16, 2016

homosaur commented Dec 16, 2016

homosaur commented Dec 16, 2016

lpinca commented Dec 16, 2016

lpinca commented Dec 16, 2016

homosaur commented Dec 16, 2016

lpinca commented Dec 16, 2016

lpinca Dec 16, 2016

Choose a reason for hiding this comment

jasnell commented Dec 23, 2016

Trott commented Dec 23, 2016

jasnell commented Dec 27, 2016

MylesBorins commented Jan 23, 2017

homosaur commented Dec 8, 2016 •

edited

Loading

Trott commented Dec 8, 2016 •

edited

Loading

ChALkeR left a comment •

edited

Loading