test: reach res._dump after abort ClientRequest #24191

tadhgcreedon · Nov 6, 2018

Adds test coverage to lib/_http_client.js by reaching a line of code inside a callback that gets registered on the 'response' event after aborting a client request, by emitting that event.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

addaleax · Nov 10, 2018

@tadhgcreedon I’m not sure, but would it maybe make sense to test both cases here, e.g. by creating a new test file rather than modifying an existing one? I think this might be affecting the previous functionality of the test…

(/cc @nodejs/http)

jasnell · Nov 10, 2018

Yeah, a separate new test file may be better

tadhgcreedon · Nov 11, 2018

Sure, makes sense. Will create a new test file then update.

oyyd · 2018-11-13T07:58:55Z

oyyd approved these changes

Nov 13, 2018

View changes

oyyd · Nov 13, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18572/

trivikr · Nov 13, 2018

Hi @tadhgcreedon, the author of the commits in this PR is not registered on GitHub (steps defined here)

Either:

add the email ID associated with the commit to GitHub
change the author of the commit to current email ID registered with the GitHub by following these steps


      test: reach res._dump after abort ClientRequest


      move changes to new test file

Trott · Nov 14, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18594/

oyyd · Nov 14, 2018

Resume: https://ci.nodejs.org/job/node-test-commit-windows-fanned/22295/

Trott · Nov 14, 2018

@addaleax LGTY?

thefourtheye · 2018-11-20T03:17:10Z

thefourtheye reviewed

Nov 20, 2018

View changes

thefourtheye · Nov 20, 2018

test/parallel/test-http-client-abort-response-event.js

+
+  req.end();
+  req.abort();
+  req.abort();


Havn't checked this completely, but two req.aborts are intentional?

/ping @tadhgcreedon Two abort() calls like that are intentional? If so, great. If not, can you remove one? Or I can totally remove one. Either way. I just want to see this get merged....


      fixup! remove extra `.abort()` call

Trott · Nov 28, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18987/

Trott · 2018-11-28T06:08:40Z

Trott approved these changes

Nov 28, 2018

View changes

Trott · Nov 29, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19013/


      test: reach res._dump after abort ClientRequest

PR-URL: nodejs#24191
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · Nov 29, 2018

Landed in f5b8853.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)


      test: reach res._dump after abort ClientRequest

PR-URL: #24191
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>


      test: reach res._dump after abort ClientRequest

PR-URL: nodejs#24191
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>


      test: reach res._dump after abort ClientRequest

PR-URL: #24191
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>


      test: reach res._dump after abort ClientRequest

PR-URL: #24191
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the test label Nov 6, 2018

addaleax added http code-and-learn labels Nov 6, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 73a1820 to 4359904 Nov 7, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 3179181 to 1a544be Nov 11, 2018

tadhgcreedon added 2 commits Nov 6, 2018

test: reach res._dump after abort ClientRequest

e3282ba

move changes to new test file

Loading status checks…

c37a91d

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 1a544be to c37a91d Nov 13, 2018

fixup! remove extra `.abort()` call

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

d8631d0

Trott added a commit to Trott/io.js that referenced this pull request Nov 29, 2018

test: reach res._dump after abort ClientRequest
PR-URL: nodejs#24191 Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

f5b8853

Trott closed this Nov 29, 2018

targos added a commit that referenced this pull request Nov 29, 2018

BridgeAR referenced this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 of 4 tasks complete

This was referenced Dec 8, 2018

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265

Closed

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

test: reach res._dump after abort ClientRequest
PR-URL: nodejs#24191 Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

4e20fbf

BethGriggs added a commit that referenced this pull request Feb 12, 2019

BethGriggs added the land-on-v10.x label Feb 12, 2019

BethGriggs referenced this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

rvagg added a commit that referenced this pull request Feb 28, 2019

test: reach res._dump after abort ClientRequest
PR-URL: #24191 Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

a06d19f

nodejs/node

test: reach res._dump after abort ClientRequest #24191

test: reach res._dump after abort ClientRequest #24191

tadhgcreedon commented Nov 6, 2018 •

edited

nodejs-github-bot added the test label Nov 6, 2018

addaleax added http code-and-learn labels Nov 6, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `73a1820` to `4359904` Nov 7, 2018

This comment has been minimized.

addaleax commented Nov 10, 2018

This comment has been minimized.

jasnell commented Nov 10, 2018

This comment has been minimized.

tadhgcreedon commented Nov 11, 2018 •

edited

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `3179181` to `1a544be` Nov 11, 2018

This comment has been minimized.

oyyd commented Nov 13, 2018

This comment has been minimized.

trivikr commented Nov 13, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `1a544be` to `c37a91d` Nov 13, 2018

This comment has been minimized.

Trott commented Nov 14, 2018

This comment has been minimized.

oyyd commented Nov 14, 2018

This comment has been minimized.

Trott commented Nov 14, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Nov 28, 2018

This comment has been minimized.

Trott commented Nov 29, 2018

This comment has been minimized.

Trott commented Nov 29, 2018

Trott closed this Nov 29, 2018

BethGriggs added the land-on-v10.x label Feb 12, 2019

nodejs/node

Join GitHub today

test: reach res._dump after abort ClientRequest #24191

Conversation

tadhgcreedon commented Nov 6, 2018 • edited

Checklist

nodejs-github-bot added the test label Nov 6, 2018

addaleax added http code-and-learn labels Nov 6, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 73a1820 to 4359904 Nov 7, 2018

This comment has been minimized.

addaleax commented Nov 10, 2018

This comment has been minimized.

jasnell commented Nov 10, 2018

This comment has been minimized.

tadhgcreedon commented Nov 11, 2018 • edited

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 3179181 to 1a544be Nov 11, 2018

This comment has been minimized.

oyyd commented Nov 13, 2018

This comment has been minimized.

trivikr commented Nov 13, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from 1a544be to c37a91d Nov 13, 2018

This comment has been minimized.

Trott commented Nov 14, 2018

This comment has been minimized.

oyyd commented Nov 14, 2018

This comment has been minimized.

Trott commented Nov 14, 2018

This comment has been minimized.

thefourtheye Nov 20, 2018

This comment has been minimized.

Trott Nov 22, 2018

This comment has been minimized.

Trott commented Nov 28, 2018

This comment has been minimized.

Trott commented Nov 29, 2018

This comment has been minimized.

Trott commented Nov 29, 2018

Trott closed this Nov 29, 2018

BethGriggs added the land-on-v10.x label Feb 12, 2019

tadhgcreedon commented Nov 6, 2018 •

edited

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `73a1820` to `4359904` Nov 7, 2018

tadhgcreedon commented Nov 11, 2018 •

edited

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `3179181` to `1a544be` Nov 11, 2018

tadhgcreedon force-pushed the tadhgcreedon:http-client-request-abort-response-dump branch from `1a544be` to `c37a91d` Nov 13, 2018