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: clean tls-connect-given-socket #8616

Closed
wants to merge 4 commits into from
Closed

test: clean tls-connect-given-socket #8616

wants to merge 4 commits into from

Conversation

thomasvanlankveld
Copy link

@thomasvanlankveld thomasvanlankveld commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
Description of change

Changed vars to consts and lets, assert.equals to assert.strictEquals and added
common.mustCall around a connect callback

Changed vars to consts and lets, assert.equals to assert.strictEquals and added
common.mustCall around a connect callback
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Sep 17, 2016

function next() {
// Already connected socket
var connected = net.connect(server.address().port, function() {
const connected = net.connect(server.address().port, function() {
establish(connected);
});
Copy link
Member

Choose a reason for hiding this comment

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

Could/should this function get a common.mustCall, too?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

rejectUnauthorized: false,
socket: socket
}, function() {
clientConnected++;
var data = '';
let data = '';
client.on('data', function(chunk) {
data += chunk.toString();
});
client.on('end', function() {
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall()?

Copy link
Author

Choose a reason for hiding this comment

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

Yes actually (35 & 38)

Thomas van Lankveld added 3 commits September 18, 2016 12:42
Changed vars to consts and lets, assert.equals to assert.strictEquals and added
common.mustCall around a connect callback
Added more common.mustCalls, switched to arrow functions
Merge branch 'clean-test-tls-connect-given-socket' of github.com:thomasvanlankveld/node into clean-test-tls-connect-given-socket
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller imyller self-assigned this Sep 20, 2016
@imyller
Copy link
Member

imyller commented Sep 20, 2016

@addaleax are you satisfied with the changes? Looks like your review suggestions have been addressed.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Yes, LGTM!

Also, I generally trust collaborators here to be able to tell whether my requested changes have been made – I appreciate the ping, but you don’t need to wait for me or anything.

});
assert(client.readable);
assert(client.writable);

return client;
}

const { port } = server.address();
Copy link
Member

Choose a reason for hiding this comment

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

Nice trick, I should think of that the next time I do this.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

I'll start landing this:

  • Four LGTMs
  • No objections
  • All requested changes to the code have been made
  • CI tests passed (usual CI failures only)

imyller pushed a commit to imyller/node that referenced this pull request Sep 20, 2016
Changed vars to consts and lets, assert.equals to
assert.strictEquals and added common.mustCall around callbacks.
Switched to arrow functions.

PR-URL: nodejs#8616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 20, 2016

landed in 80620d8

Thank you for your contribution, @thomasvanlankveld

@imyller imyller closed this Sep 20, 2016
@imyller imyller removed their assignment Sep 20, 2016
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Changed vars to consts and lets, assert.equals to
assert.strictEquals and added common.mustCall around callbacks.
Switched to arrow functions.

PR-URL: #8616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants