🌐 US-Proxy
class="logged-out env-production page-responsive" style="word-wrap: break-word;" >
Skip to content

test: make HTTP/1.0 connection test more robust#55959

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
FliegendeWurst:patch-1
Nov 24, 2024
Merged

test: make HTTP/1.0 connection test more robust#55959
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
FliegendeWurst:patch-1

Conversation

@FliegendeWurst

Copy link
Copy Markdown
Contributor

Fixes: #47200

This is a more robust alternative to #47316

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 22, 2024
Comment thread test/parallel/test-http-remove-connection-header-persists-connection.js Outdated
'Host: localhost:' + server.address().port + '\r\n' +
'\r\n');
socket.resume(); // Ignore the response itself
socket.end(); // we are done sending data

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
socket.end(); // we are done sending data

This invalidate the test. The socket should be closed by the server.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This only closes one end of the socket, as I understand it. But I will remove it.

Comment thread test/parallel/test-http-remove-connection-header-persists-connection.js Outdated
Comment thread test/parallel/test-http-remove-connection-header-persists-connection.js Outdated
Comment thread test/parallel/test-http-remove-connection-header-persists-connection.js Outdated
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@github-actions github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 23, 2024
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/11986219813

@FliegendeWurst

Copy link
Copy Markdown
Contributor Author

(lint failed due to unused common)

@lpinca

lpinca commented Nov 23, 2024

Copy link
Copy Markdown
Member

Use require('../common');. Do not remove it completely, it is mandatory.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@FliegendeWurst

Copy link
Copy Markdown
Contributor Author

parallel.test-http-server-headers-timeout-keepalive failed. I don't see how it relates to this change.

@codecov

codecov Bot commented Nov 23, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (3178a76) to head (401f5f2).
Report is 560 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #55959    +/-   ##
========================================
  Coverage   87.99%   87.99%            
========================================
  Files         653      653            
  Lines      187852   188091   +239     
  Branches    35888    35945    +57     
========================================
+ Hits       165295   165514   +219     
- Misses      15729    15742    +13     
- Partials     6828     6835     +7     

see 29 files with indirect coverage changes

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lpinca

lpinca commented Nov 23, 2024

Copy link
Copy Markdown
Member

@FliegendeWurst to actually verify that the socket is closed immediately after sending the response, I would refactor the test as follows

diff --git a/test/parallel/test-http-remove-connection-header-persists-connection.js b/test/parallel/test-http-remove-connection-header-persists-connection.js
index df7e39ae94..4afb863c44 100644
--- a/test/parallel/test-http-remove-connection-header-persists-connection.js
+++ b/test/parallel/test-http-remove-connection-header-persists-connection.js
@@ -10,6 +10,13 @@ const server = http.createServer(function(request, response) {
   // For HTTP/1.0, the connection should be closed after the response automatically.
   response.removeHeader('connection');
 
+  if (request.httpVersion === '1.0') {
+    const socket = request.socket;
+    response.on('finish', common.mustCall(function() {
+      assert.ok(socket.writableEnded);
+    }));
+  }
+
   response.end('beep boop\n');
 });
 
@@ -49,10 +56,7 @@ function makeHttp10Request(cb) {
                'Host: localhost:' + server.address().port + '\r\n' +
                 '\r\n');
     socket.resume(); // Ignore the response itself
-
-    setTimeout(function() {
-      cb(socket);
-    }, common.platformTimeout(50));
+    socket.on('close', cb);
   });
 }
 
@@ -62,9 +66,7 @@ server.listen(0, function() {
       // Both HTTP/1.1 requests should have used the same socket:
       assert.strictEqual(firstSocket, secondSocket);
 
-      makeHttp10Request(function(socket) {
-        // The server should have immediately closed the HTTP/1.0 socket:
-        assert.strictEqual(socket.closed, true);
+      makeHttp10Request(function() {
         server.close();
       });
     });

Fixes: nodejs#47200

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@FliegendeWurst

Copy link
Copy Markdown
Contributor Author

Done as suggested. I also checked the http server: the socket should be closed before our 'finish' handler is called.

@lpinca lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky Test parallel/test-http-remove-connection-header-persists-connection

6 participants