http: Add coverage for socketOnDrain and updateOngoingData #17051

Activity

1. mscdex

   added

   httpIssues and PRs related to the http subsystem.Issues and PRs related to the http subsystem.

   testIssues and PRs related to Node.js core tests and test infrastructure.Issues and PRs related to Node.js core tests and test infrastructure.

   on Nov 15, 2017
2. sashaaKr commented on Nov 20, 2017

   

   sashaaKr

   on Nov 20, 2017

   More actions

   Hey, I'm going to work on it
3. Leko commented on Dec 11, 2017

   

   Leko

   on Dec 11, 2017

   Contributor

   More actions

   @mcollina It seems to covered in latest coverage report.
   https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/_http_server.js.html

   But I cannot find test case about socketOnDrain.
   This function is covered indirectly by another function tests.

   $ git grep socketOnDrain
   lib/_http_server.js:342:  state.onDrain = socketOnDrain.bind(undefined, socket, state);
   lib/_http_server.js:377:    return socketOnDrain(socket, state);
   lib/_http_server.js:381:function socketOnDrain(socket, state) {
   

   Should we write a test according to socketOnDrain ?
4. mcollina commented on Dec 11, 2017

   

   mcollina

   on Dec 11, 2017

   SponsorMemberAuthor

   More actions

   our test suite does not check when needPause becomes false. We should test all the combinations for the various conditionals in socketOnDrain.
5. Leko commented on Dec 11, 2017

   

   Leko

   on Dec 11, 2017

   Contributor

   More actions

   We should test all the combinations for the various conditionals in socketOnDrain.

   @mcollina Thank you for the detailed explanation.
   I got it. I'll try to write test.
6. mcollina commented on Dec 15, 2017

   

   mcollina

   on Dec 15, 2017

   SponsorMemberAuthor

   More actions

   Fixed in a364e7e.
7. mcollina

   closed this as completedon Dec 15, 2017
8. Leko commented on Dec 15, 2017

   

   Leko

   on Dec 15, 2017

   Contributor

   More actions

   Thank you for your quick response :D