test: convert var->const/let in tests by gibfahn · Pull Request #10685 · nodejs/node · GitHub

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

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?

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

Add the rule to test/.eslintrc.

@targos

Add the rule to test/.eslintrc.

Thanks, done

Rubber stamp LGTM if the CI passes.

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

EDIT: CI passed

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

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

Rubber-stamp LGTM

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.

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

EDIT: Still green

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

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.

unneccesary wrapping?

The semicolon puts it over 80 chars 😭

unneccesary wrapping?

81 chars again

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

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

unneccesary wrapping?

as above

unneccesary wrapping?

as above

Same here, can probably use const.

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

const again.

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.

Maybe alphabetically sort the rules?

Will do

@silverwind Updated, PTAL

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>

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

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.

v4.x backport: #10685

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

@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 pushed a commit that referenced this pull request Apr 13, 2017

tools, test: require const/let in test

daddf5a

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>

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017

tools, test: require const/let in test

de63698

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>

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 by gibfahn · Pull Request #10685 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: convert var->const/let in tests#10685gibfahn merged 2 commits intonodejs:masternodejs/node:masterfrom gibfahn:var2constgibfahn/node:var2constCopy head branch name to clipboard

Conversation

gibfahn commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

To fix rules with eslint

Checklist

Affected core subsystem(s)

Uh oh!

gibfahn commented Jan 8, 2017

Uh oh!

targos commented Jan 8, 2017

Uh oh!

gibfahn commented Jan 8, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Jan 8, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jan 9, 2017

Uh oh!

gibfahn commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

test: convert var->const/let in tests#10685
gibfahn merged 2 commits into
nodejs:masterfrom
gibfahn:var2const

gibfahn commented Jan 8, 2017 •
edited
Loading

gibfahn commented Jan 8, 2017 •
edited
Loading

gibfahn commented Jan 10, 2017 •
edited
Loading