test: wrap callbacks in mustCall() for test-http-agent-destroyed-socket.js #11201

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@shubheksha
Contributor

shubheksha commented Feb 6, 2017

Wrap the callbacks which make assertions in common.mustcall() to ensure they are called

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

Tests for http

test: wrap callbacks in mustCall()
Wrap the callbacks which make assertions in common.mustcall() to ensure they are called
@Trott

Trott approved these changes Feb 6, 2017

LGTM if CI is green. Would prefer to not add the two new console.log() statements if there isn't a super-compelling reason to do so.

@@ -45,20 +45,22 @@ const server = http.createServer(function(req, res) {
request1.socket.destroy();
response.once('close', function() {
+ console.log('called');

This comment has been minimized.

@Trott

Trott Feb 6, 2017

Member

Nit: remove debugging addition?

@Trott

Trott Feb 6, 2017

Member

Nit: remove debugging addition?

// assert that the same socket was not assigned to request2,
// since it was destroyed.
+ console.log('called 2');

This comment has been minimized.

@Trott

Trott Feb 6, 2017

Member

Nit: remove this one too?

@Trott

Trott Feb 6, 2017

Member

Nit: remove this one too?

@Trott Trott added the http label Feb 6, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig approved these changes Feb 6, 2017

LGTM, but it looks like one more could be added on line 47 (response.once('close').

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 6, 2017

Member

LGTM, but it looks like one more could be added on line 47 (response.once('close').

@cjihrig That callback does not execute at all when I run the test. (Should it? If so, I think there's a bug.) Perhaps it is there to either handle a race condition or else is platform-dependent? The big comment a few lines above it seems relevant.

Member

Trott commented Feb 6, 2017

LGTM, but it looks like one more could be added on line 47 (response.once('close').

@cjihrig That callback does not execute at all when I run the test. (Should it? If so, I think there's a bug.) Perhaps it is there to either handle a race condition or else is platform-dependent? The big comment a few lines above it seems relevant.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 7, 2017

Contributor

Yea, that comment does seem relevant. It should be fine to leave line 47 alone since it might not execute. It seems less than ideal to have cases like that in our tests, but that has nothing to do with this PR.

Contributor

cjihrig commented Feb 7, 2017

Yea, that comment does seem relevant. It should be fine to leave line 47 alone since it might not execute. It seems less than ideal to have cases like that in our tests, but that has nothing to do with this PR.

@Trott

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Feb 11, 2017

test: improve test-http-agent-destroyed-socket.js
* wrap callbacks in mustCall()
* Wrap the callbacks which make assertions in common.mustcall()
  to ensure they are called

PR-URL: #11201
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 11, 2017

Member

Landed in 3d2bd7a

Member

jasnell commented Feb 11, 2017

Landed in 3d2bd7a

@jasnell jasnell closed this Feb 11, 2017

italoacasas added a commit that referenced this pull request Feb 13, 2017

test: improve test-http-agent-destroyed-socket.js
* wrap callbacks in mustCall()
* Wrap the callbacks which make assertions in common.mustcall()
  to ensure they are called

PR-URL: #11201
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: improve test-http-agent-destroyed-socket.js
* wrap callbacks in mustCall()
* Wrap the callbacks which make assertions in common.mustcall()
  to ensure they are called

PR-URL: nodejs#11201
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: improve test-http-agent-destroyed-socket.js
* wrap callbacks in mustCall()
* Wrap the callbacks which make assertions in common.mustcall()
  to ensure they are called

PR-URL: nodejs#11201
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

needs a backport PR to land in v6 or v4

Member

jasnell commented Mar 7, 2017

needs a backport PR to land in v6 or v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment