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: added net.connect lookup type check #11873

Closed

Conversation

@lucamaraschi
Copy link
Contributor

commented Mar 15, 2017

Check the options passed to the Socket connect function to validate
the type of the lookup property. It must be strictly a function.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test net

test: added net.connect lookup type check
Check the options passed to the connect function of Socket to validate
the type of the lookupmproperty. It must be strictly a function.
@jasnell
Copy link
Member

left a comment

LGTM. Few nits.


const expectedError = /^TypeError: "lookup" option should be a function$/;

['foobar', 1, {}, []].forEach((input => { connectThrows(input); }))

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 15, 2017

Member

missing a ; at the end

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 15, 2017

Member

Also.. the { and ; } around connectThrows(input) can be dropped.

['foobar', 1, {}, []].forEach((input => { connectThrows(input); }))

function connectThrows(input) {
var opts = {

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 15, 2017

Member

s/var/const

@@ -5,10 +5,10 @@ const net = require('net');

const expectedError = /^TypeError: "lookup" option should be a function$/;

['foobar', 1, {}, []].forEach((input => { connectThrows(input); }))
['foobar', 1, {}, []].forEach((input => connectThrows(input) ));

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 16, 2017

Contributor

Can the extra parens around the arrow function here and on line 22 be removed?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

@mscdex mscdex added the net label Mar 16, 2017

const opts = {
host: 'localhost',
port: common.PORT,
lookup: input,

This comment has been minimized.

Copy link
@mcollina

mcollina Mar 16, 2017

Member

you should remove the comma here. Is this passing make lint?

const opts = {
host: 'localhost',
port: common.PORT,
lookup: input,

This comment has been minimized.

Copy link
@mcollina

mcollina Mar 16, 2017

Member

same here


const expectedError = /^TypeError: "lookup" option should be a function$/;

['foobar', 1, {}, []].forEach((input) => connectThrows(input));

This comment has been minimized.

Copy link
@lpinca

lpinca Mar 17, 2017

Member

Nit: how about adding other primitives (boolean, undefined, symbol)?

@lpinca
lpinca approved these changes Mar 17, 2017
@jasnell

This comment has been minimized.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2017

Landed in 1ff6796. Thanks.

@cjihrig cjihrig closed this Mar 20, 2017

cjihrig added a commit that referenced this pull request Mar 20, 2017
test: added net.connect lookup type check
Check the options passed to Socket.prototype.connect() to
validate the type of the lookup property.

PR-URL: #11873
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
test: added net.connect lookup type check
Check the options passed to Socket.prototype.connect() to
validate the type of the lookup property.

PR-URL: nodejs#11873
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

This PR need backport to v7

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Landed cleanly on v6.x (it's a new file, not sure why it needed a backport to v7).

gibfahn added a commit that referenced this pull request Jun 20, 2017
test: added net.connect lookup type check
Check the options passed to Socket.prototype.connect() to
validate the type of the lookup property.

PR-URL: #11873
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: added net.connect lookup type check
Check the options passed to Socket.prototype.connect() to
validate the type of the lookup property.

PR-URL: #11873
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.