Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: removed unnecessary Buffer import #13860

Closed
wants to merge 5 commits into from

Conversation

swinston1000
Copy link
Contributor

@swinston1000 swinston1000 commented Jun 21, 2017

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

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

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits

@@ -8,7 +8,7 @@
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// The above copnotice and this permission notice shall be included
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed :-)

@@ -8,7 +8,7 @@
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// The above copnotice and this permission notice shall be included
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please revert this change?

const child_process = require('child_process');
const fs = require('fs');
const net = require('net');
var netBinding = process.binding('net');
const path = require('path');

var DATA = {
'ppid' : process.pid,
'ord' : 0
'ppid': process.pid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Something like:

test: removed unnecessary require and style fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated style change.

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'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@swinston1000 swinston1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -8,7 +8,7 @@
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// The above copnotice and this permission notice shall be included
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@aqrln
Copy link
Contributor

aqrln commented Jun 21, 2017

@@ -8,7 +8,7 @@
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// The above copnotice and this permission notice shall be included
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an accident :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'twas, now reverted

const child_process = require('child_process');
const fs = require('fs');
const net = require('net');
var netBinding = process.binding('net');
const path = require('path');

var DATA = {
'ppid' : process.pid,
'ord' : 0
'ppid': process.pid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the formatting changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now reverted :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and thanks :)

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@tniessen tniessen self-assigned this Jun 22, 2017
tniessen pushed a commit that referenced this pull request Jun 23, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@tniessen
Copy link
Member

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

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

@tniessen tniessen closed this Jun 23, 2017
addaleax pushed a commit that referenced this pull request Jun 24, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@addaleax addaleax mentioned this pull request Jun 24, 2017
addaleax pushed a commit that referenced this pull request Jun 29, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.