test: removed unnecessary Buffer import by swinston1000 · Pull Request #13860 · nodejs/node

swinston1000 · 2017-06-21T21:02:00Z

no need to require('buffer') in test files, so removed from:

test\disabled\test-sendfd.js
test\internet\test-dgram-broadcast-multi-process.js
test\pummel\test-https-no-reader.js

first ever contribution :-)
thanks to #goodnessSquad for the support and vision!

Refs: #13836

Checklist

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

Affected core subsystem(s)

test

refack

Looks good, just a few nits

refack · 2017-06-21T21:10:03Z

test/disabled/test-sendfd.js

 // following conditions:
 //
-// The above copyright notice and this permission notice shall be included
+// The above copnotice and this permission notice shall be included


Unintentional?
(beware of over eager IDEs. If you're using WebStorm, enable ESlint with our .eslint.yaml)

yes completely, do you want me to close and fix all of this?

refack · 2017-06-21T21:12:06Z

test/disabled/test-sendfd.js

 var SOCK_PATH = path.join(__dirname,
-                          '..',
-                          path.basename(__filename, '.js') + '.sock');
+  '..',


So we either chop (put every arg in it's own line)
Or align to call ( (as it was).

aqrln · 2017-06-21T21:05:00Z

test/disabled/test-sendfd.js

 // following conditions:
 //
-// The above copyright notice and this permission notice shall be included
+// The above copnotice and this permission notice shall be included


Can you please revert this change?

aqrln · 2017-06-21T21:09:21Z

test/disabled/test-sendfd.js

 var DATA = {
-  'ppid' : process.pid,
-  'ord' : 0
+  'ppid': process.pid,


This change is unrelated. You may do it in this PR if you want, but you need to reframe the commit message and the PR title differently then.

Suggestion: Something like:

test: removed unnecessary require and style fix

Reverted the change so won't change title
Thanks for pointing it out
Lesson learned not to have auto-beautify when working on open-source projects :-)

aqrln · 2017-06-21T21:09:55Z

test/disabled/test-sendfd.js

 var SOCK_PATH = path.join(__dirname,
-                          '..',
-                          path.basename(__filename, '.js') + '.sock');
+  '..',


Unrelated style change.

aqrln · 2017-06-21T21:14:16Z

test/disabled/test-sendfd.js

-var cp = child_process.spawn(process.argv[0],
-                             [path.join(common.fixturesDir, 'recvfd.js'),
-                              SOCK_PATH]);
+var cp = child_process.spawn(process.argv[0], [path.join(common.fixturesDir, 'recvfd.js'),


Unrelated style change. Besides that, it will fail linting. Please don't ignore the friendly reminder to run make -j4 test you can see in the pull request template :)

I did, and it failed first time
I changed, it passed
Not sure how this happened
Fixing now, sorry :-)

swinston1000

Sorry, my first ever contribution and didn't fully appreciate the style guide and the fact my IDE had made so many changes. All should be OK now. Thanks for your patience.

swinston1000 · 2017-06-21T21:13:50Z

test/disabled/test-sendfd.js

 // following conditions:
 //
-// The above copyright notice and this permission notice shall be included
+// The above copnotice and this permission notice shall be included


yes completely, do you want me to close and fix all of this?

swinston1000 · 2017-06-21T21:19:38Z

test/disabled/test-sendfd.js

-var cp = child_process.spawn(process.argv[0],
-                             [path.join(common.fixturesDir, 'recvfd.js'),
-                              SOCK_PATH]);
+var cp = child_process.spawn(process.argv[0], [path.join(common.fixturesDir, 'recvfd.js'),


I did, and it failed first time
I changed, it passed
Not sure how this happened
Fixing now, sorry :-)

aqrln

LGTM, thanks!

aqrln · 2017-06-21T21:24:34Z

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

benjamingr · 2017-06-21T21:09:45Z

test/disabled/test-sendfd.js

 // following conditions:
 //
-// The above copyright notice and this permission notice shall be included
+// The above copnotice and this permission notice shall be included


This looks like an accident :)

'twas, now reverted

benjamingr · 2017-06-21T21:09:58Z

test/disabled/test-sendfd.js

 var DATA = {
-  'ppid' : process.pid,
-  'ord' : 0
+  'ppid': process.pid,


Why the formatting changes?

@benjamingr

Auto beautifier - and the tests didn't pick it up.
I reverted all changes - should all be good now and know in future to turn that off or use your guide!

Sorry and thanks for this evening!

now reverted :-)

benjamingr

LGTM and thanks :)

refack

🥇

Removed require('buffer') from - test/disabled/test-sendfd.js - test/internet/test-dgram-broadcast-multi-process.js - test/pummel/test-https-no-reader.js PR-URL: #13860 Refs: #13836 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

tniessen · 2017-06-23T22:52:48Z

Landed in e42b1b1, thank you for your first contribution! 🎉

CI on master: https://ci.nodejs.org/job/node-test-commit/10749/

Removed require('buffer') from - test/disabled/test-sendfd.js - test/internet/test-dgram-broadcast-multi-process.js - test/pummel/test-https-no-reader.js PR-URL: #13860 Refs: #13836 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

test: removed unnecessary Buffer import

4b612f6

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 21, 2017

refack reviewed Jun 21, 2017

View reviewed changes

aqrln reviewed Jun 21, 2017

View reviewed changes

Update test-sendfd.js

e903a96

refack mentioned this pull request Jun 21, 2017

test: remove unnecessary Buffer imports #13836

Closed

swinston1000 added 3 commits June 22, 2017 00:17

reverted code

daa9328

reverted style change

ac96ae9

reverted style change

9e2e60d

swinston1000 commented Jun 21, 2017

View reviewed changes

aqrln approved these changes Jun 21, 2017

View reviewed changes

jasnell approved these changes Jun 21, 2017

View reviewed changes

benjamingr approved these changes Jun 21, 2017

View reviewed changes

vsemozhetbyt approved these changes Jun 21, 2017

View reviewed changes

benjamingr approved these changes Jun 21, 2017

View reviewed changes

refack approved these changes Jun 21, 2017

View reviewed changes

TimothyGu approved these changes Jun 22, 2017

View reviewed changes

tniessen self-assigned this Jun 22, 2017

tniessen approved these changes Jun 22, 2017

View reviewed changes

cjihrig approved these changes Jun 22, 2017

View reviewed changes

mhdawson approved these changes Jun 23, 2017

View reviewed changes

tniessen closed this Jun 23, 2017

addaleax mentioned this pull request Jun 24, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the land-on-v6.x label Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Uh oh!

Conversation

swinston1000 commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

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

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!

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

aqrln left a comment

Choose a reason for hiding this comment

Uh oh!

aqrln commented Jun 21, 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!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jun 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

swinston1000 commented Jun 21, 2017 •

edited

Loading