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: use common.mustCall in test-tls-connect-given-socket.js #12592

Closed
wants to merge 2 commits into from

Conversation

@vperezma
Copy link
Contributor

commented Apr 22, 2017

remove process block and add common.mustCall

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

@@ -72,14 +66,14 @@ const server = tls.createServer(options, (socket) => {

// Immediate death socket
const immediateDeath = net.connect(port);
establish(immediateDeath).destroy();
establish(immediateDeath, 0).destroy();

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 22, 2017

Contributor

I don't think it makes sense to pass 0 to common.mustCall() as the expected argument. I would expect any common.mustCall() to be passed a value of 1 (or undefined) or higher. If the intention is that it should never be called, common.mustNotCall() should be used instead.

This comment has been minimized.

Copy link
@Trott

Trott Apr 22, 2017

Member

undefined will result in the default of 1. I think either we need to be OK with sending 0 to common.mustCall() (I am, at least in this case) or else add some logic to switch out whether common.mustCall() or common.mustNotCall() gets called. That works too, although might be a bit much. I'm fine with either.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 24, 2017

Member

This one was a bit of let's get it working. establish() is called multiple times with different expected results each time and this was the easiest to get things going. The test as written, however, could use a bit of a bigger refactoring.

@mscdex mscdex added the tls label Apr 22, 2017

@jasnell jasnell changed the title Change socket tls test: use common.mustCall in test-tls-connect-given-socket.js Apr 24, 2017

@Trott
Trott approved these changes May 7, 2017
Copy link
Member

left a comment

LGTM if CI is green

@jasnell
jasnell approved these changes May 9, 2017
Trott added a commit to Trott/io.js that referenced this pull request May 15, 2017
test: use mustCall in tls-connect-given-socket
PR-URL: nodejs#12592
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Landed in 72e3dda.
Thanks for the contribution! 🎉

@Trott Trott closed this May 15, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: use mustCall in tls-connect-given-socket
PR-URL: nodejs#12592
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Jun 22, 2017
test: use mustCall in tls-connect-given-socket
PR-URL: #12592
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: use mustCall in tls-connect-given-socket
PR-URL: #12592
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@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
7 participants
You can’t perform that action at this time.