test: convert var->const/let in tests #10685

gibfahn · 2017-01-08T14:59:24Z

Overview

Use eslint to convert all var to const/let in test/, manually fix anything that eslint messed up.

If we're going to go ES6 in test/, we might as well go all the way.

To fix rules with eslint

Apply this:

diff --git a/.eslintrc b/.eslintrc
index cf1f36c86b..e09fd6ac64 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -110,6 +110,8 @@ rules:
 
   # ECMAScript 6
   # http://eslint.org/docs/rules/#ecmascript-6
+  no-var: error
+  prefer-const: error
   arrow-parens: [2, always]
   arrow-spacing: [2, {before: true, after: true}]
   constructor-super: 2
@@ -119,7 +121,6 @@ rules:
   no-dupe-class-members: 2
   no-new-symbol: 2
   no-this-before-super: 2
-  prefer-const: [2, {ignoreReadBeforeAssign: true}]
   rest-spread-spacing: 2
   template-curly-spacing: 2

Run this:

eslint --fix --rulesdir=tools/eslint-rules "test/**/*.js"

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

gibfahn · 2017-01-08T15:01:25Z

cc/ @nodejs/testing

cc/ @silverwind @Trott @not-an-aardvark Is there an easy way to make rules only apply to test/? Also should @not-an-aardvark be on the tools/eslint cc list in onboarding-extras?

targos · 2017-01-08T15:02:28Z

Is there an easy way to make rules only apply to test/

Add the rule to test/.eslintrc.

gibfahn · 2017-01-08T15:25:19Z

@targos

Add the rule to test/.eslintrc.

Thanks, done

cjihrig

Rubber stamp LGTM if the CI passes.

gibfahn · 2017-01-08T22:29:14Z

CI: https://ci.nodejs.org/job/node-test-commit/7103/

EDIT: CI passed

gibfahn · 2017-01-08T23:03:51Z

It's probably worth checking the second and third commits:

second (manual fixups of eslint issues): 27d10c0
third (require let/const in test/): 21725bf

jasnell

Rubber-stamp LGTM

gibfahn · 2017-01-09T08:03:29Z

test/.eslintrc

@@ -4,3 +4,5 @@ rules:
  ## common module is mandatory in tests
  required-modules: [2, common]
  prefer-assert-methods: 2
+  no-var: error
+  prefer-const: error


Should these be:

no-var: 2 prefer-const: 2

? I'm not sure what the 2 represents here.

2 means error

Changed error to 2 for consistency.

gibfahn · 2017-01-09T13:17:55Z

CI 2: https://ci.nodejs.org/job/node-test-commit/7112/

EDIT: Still green

gibfahn · 2017-01-10T22:46:05Z

CI 3: https://ci.nodejs.org/job/node-test-commit/7158/

silverwind · 2017-01-10T22:53:03Z

test/parallel/test-http-get-pipeline-problem.js

@@ -35,7 +35,8 @@ const server = http.Server(function(req, res) {


 server.listen(0, function() {
-  for (var i = 0; i < total; i++) {
+  let i;
+  for (i = 0; i < total; i++) {


I think you can const this one.

➜ node git:(var2const) tools/test.py test/parallel/test-http-get-pipeline-problem.js === release test-http-get-pipeline-problem === Path: parallel/test-http-get-pipeline-problem image.length = 45658 /Users/gib/wrk/com/node/test/parallel/test-http-get-pipeline-problem.js:38 for (const i = 0; i < total; i++) { ^ TypeError: Assignment to constant variable.

If this is what you meant, I don't think so. I can do for (let i = 0; though.

Weird, I could swear const loop variables once worked, but I see it's failing now. Your let suggestion sounds good.

silverwind · 2017-01-10T22:54:18Z

test/parallel/test-http-many-ended-pipelines.js

@@ -28,7 +28,8 @@ const server = http.createServer(function(req, res) {
 });

 server.listen(0, function() {
-  const client = net.connect({ port: this.address().port, allowHalfOpen: true });
+  const client = net.connect({ port: this.address().port,
+                               allowHalfOpen: true });


unneccesary wrapping?

The semicolon puts it over 80 chars 😭

silverwind · 2017-01-10T22:54:27Z

test/parallel/test-http-url.parse-auth-with-header-in-request.js

@@ -18,7 +18,8 @@ const server = http.createServer(function(request, response) {
 });

 server.listen(0, function() {
-  const testURL = url.parse(`http://asdf:qwer@localhost:${this.address().port}`);
+  const testURL = url.parse(`http://asdf:qwer@localhost:${this.address().port}`
+      );


unneccesary wrapping?

81 chars again

Move the string to a variable so it doesn't look so hideous? :)

silverwind · 2017-01-10T22:54:47Z

test/parallel/test-net-local-address-port.js

@@ -13,7 +13,8 @@ const server = net.createServer(common.mustCall(function(socket) {
 }));

 server.listen(0, common.localhostIPv4, function() {
-  const client = net.createConnection(this.address().port, common.localhostIPv4);
+  const client = net.createConnection(this.address()
+                    .port, common.localhostIPv4);


unneccesary wrapping?

➜ node git:(var2const) ✗ make lint ./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \ benchmark lib test tools /Users/gib/wrk/com/node/test/parallel/test-net-local-address-port.js 16:1 error Line 16 exceeds the maximum line length of 80 max-len

silverwind · 2017-01-10T22:55:15Z

test/parallel/test-repl-tab.js

@@ -6,7 +6,8 @@ const zlib = require('zlib');

 // just use builtin stream inherited from Duplex
 const putIn = zlib.createGzip();
-const testMe = repl.start('', putIn, function(cmd, context, filename, callback) {
+const testMe = repl.start('', putIn, function(cmd, context, filename,
+      callback) {


unneccesary wrapping?

silverwind · 2017-01-10T22:55:52Z

test/parallel/test-vm-static-this.js

-           'if (typeof baz !== \'undefined\') throw new Error(\'test fail\');';
+             'bar = 2;' +
+             'if (typeof baz !== \'undefined\')' +
+             'throw new Error(\'test fail\');';


unneccesary wrapping?

silverwind · 2017-01-10T22:56:18Z

test/parallel/test-writeuint.js

@@ -127,7 +127,8 @@ function testUint(clazz) {
  let val = 1;

  // Test 0 to 5 bytes.
-  for (var i = 0; i <= 5; i++) {
+  let i;
+  for (i = 0; i <= 5; i++) {


Same here, can probably use const.

Changed to let as above (const didn't work)

silverwind · 2017-01-10T22:56:37Z

test/parallel/test-zlib.js

                  let ok = true;
                  const testNum = ++done;
-                  for (var i = 0; i < Math.max(c.length, test.length); i++) {
+                  let i;
+                  for (i = 0; i < Math.max(c.length, test.length); i++) {


const again.

silverwind · 2017-01-10T22:56:57Z

test/pummel/test-net-connect-econnrefused.js

@@ -15,7 +15,7 @@ pummel();
 function pummel() {
  console.log('Round', rounds, '/', ROUNDS);

-  for (var pending = 0; pending < ATTEMPTS_PER_ROUND; pending++) {
+  for (let pending = 0; pending < ATTEMPTS_PER_ROUND; pending++) {


const here too.

const doesn't work (the pending++ would be modifying it)

Actually, as is the test failed (it's not run in CI), to fix I had to move the let to the previous line.

silverwind · 2017-01-10T22:58:27Z

test/.eslintrc

@@ -5,3 +5,5 @@ rules:
  required-modules: [2, common]
  prefer-assert-iferror: 2
  prefer-assert-methods: 2
+  no-var: 2
+  prefer-const: 2


Maybe alphabetically sort the rules?

gibfahn · 2017-01-10T23:15:19Z

@silverwind Updated, PTAL

gibfahn · 2017-01-10T23:16:43Z

Actually, I just realised that the eslint rule isn't showing up all the vars for some reason, I'll investigate later.

Manually fix issues that eslint --fix couldn't do automatically. PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Manually fix issues that eslint --fix couldn't do automatically. PR-URL: nodejs#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

PR-URL: nodejs#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Manually fix issues that eslint --fix couldn't do automatically. PR-URL: nodejs#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

PR-URL: nodejs#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

jasnell · 2017-03-08T20:53:44Z

This will need backport PRs in order to land on v6 or v4

bnoordhuis · 2017-03-09T06:59:31Z

This is likely preventing a lot of pull requests from back-porting cleanly so I think it should be back-ported with some urgency. Likewise for #10698.

gibfahn · 2017-03-09T16:39:03Z

v4.x backport: #10685

MylesBorins · 2017-03-09T17:25:38Z

@gibfahn did can you do a v6.x backport too?

gibfahn · 2017-03-09T17:50:27Z

@MylesBorins I'll do it once I get the v4.x backport working. There's more pig-wrestling than I'd expected.

Backport-PR-URL: #11775 PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Manually fix issues that eslint --fix couldn't do automatically. Backport-PR-URL: #11775 PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Backport-PR-URL: #11775 PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Manually fix issues that eslint --fix couldn't do automatically. Backport-PR-URL: #11775 PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Backport-PR-URL: nodejs/node#11775 PR-URL: nodejs/node#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Manually fix issues that eslint --fix couldn't do automatically. Backport-PR-URL: nodejs/node#11775 PR-URL: nodejs/node#10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

gibfahn added the test Issues and PRs related to the tests. label Jan 8, 2017

nodejs-github-bot added the dont-land-on-v7.x label Jan 8, 2017

cjihrig approved these changes Jan 8, 2017

View reviewed changes

mscdex removed the dont-land-on-v7.x label Jan 9, 2017

jasnell approved these changes Jan 9, 2017

View reviewed changes

gibfahn commented Jan 9, 2017

View reviewed changes

gibfahn force-pushed the var2const branch from 21725bf to cec64bb Compare January 9, 2017 11:08

nodejs-github-bot added the dont-land-on-v7.x label Jan 9, 2017

gibfahn force-pushed the var2const branch from cec64bb to 18260b6 Compare January 9, 2017 13:21

nodejs-github-bot added the dont-land-on-v7.x label Jan 9, 2017

gibfahn force-pushed the var2const branch from 18260b6 to fabe275 Compare January 10, 2017 22:37

nodejs-github-bot added the dont-land-on-v7.x label Jan 10, 2017

gibfahn force-pushed the var2const branch from fabe275 to 5a1242e Compare January 10, 2017 22:45

nodejs-github-bot added the dont-land-on-v7.x label Jan 10, 2017

gibfahn force-pushed the var2const branch from 5a1242e to 6dfa97d Compare January 10, 2017 22:53

nodejs-github-bot added the dont-land-on-v7.x label Jan 10, 2017

silverwind suggested changes Jan 10, 2017

View reviewed changes

silverwind reviewed Jan 10, 2017

View reviewed changes

gibfahn force-pushed the var2const branch from 6dfa97d to d478825 Compare January 10, 2017 23:14

nodejs-github-bot added the dont-land-on-v7.x label Jan 10, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

targos pushed a commit that referenced this pull request Jan 28, 2017

tools, test: require const/let in test

06c5604

PR-URL: #10685 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

gibfahn restored the var2const branch January 29, 2017 22:30

gibfahn deleted the var2const branch January 29, 2017 22:32

gibfahn mentioned this pull request Jan 31, 2017

test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

Closed

3 tasks

jasnell added dont-land-on-v4.x labels Mar 8, 2017

davidben mentioned this pull request Mar 8, 2017

crypto: Remove unused access of tlsext_hostname. #10882

Closed

3 tasks

gibfahn restored the var2const branch March 9, 2017 08:28

gibfahn mentioned this pull request Mar 9, 2017

v4.x backport - test: convert var->const/let in tests #11773

Closed

2 tasks

gibfahn mentioned this pull request Mar 9, 2017

v6.x backport - test: convert var->const/let in tests #11775

Closed

2 tasks

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

gibfahn deleted the var2const branch June 3, 2017 14:34

test: convert var->const/let in tests #10685

test: convert var->const/let in tests #10685

Conversation

gibfahn commented Jan 8, 2017 • edited Loading

Overview

To fix rules with eslint

Checklist

Affected core subsystem(s)

gibfahn commented Jan 8, 2017

targos commented Jan 8, 2017

gibfahn commented Jan 8, 2017

cjihrig left a comment

Choose a reason for hiding this comment

gibfahn commented Jan 8, 2017 • edited Loading

gibfahn commented Jan 8, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Jan 9, 2017

gibfahn commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Jan 10, 2017

gibfahn commented Jan 10, 2017 • edited Loading

jasnell commented Mar 8, 2017

bnoordhuis commented Mar 9, 2017

gibfahn commented Mar 9, 2017

MylesBorins commented Mar 9, 2017

gibfahn commented Mar 9, 2017

gibfahn commented Jan 8, 2017 •

edited

Loading

gibfahn commented Jan 8, 2017 •

edited

Loading

gibfahn commented Jan 10, 2017 •

edited

Loading