increase test coverage of node:http (#4535)

Author: anonrig

justfile

@@ -36,7 +36,7 @@ stream-test *args:
 
 # e.g. just node-test zlib
 node-test test_name *args:
-  just stream-test //src/workerd/api/node:tests/{{test_name}}-nodejs-test {{args}}
+  just stream-test //src/workerd/api/node/tests:{{test_name}}-nodejs-test {{args}}
 
 # e.g. just wpt-test urlpattern
 wpt-test test_name:

src/node/http.ts

@@ -10,6 +10,7 @@ import {
 import { METHODS, STATUS_CODES } from 'node-internal:internal_http_constants';
 import { ClientRequest } from 'node-internal:internal_http_client';
 import { OutgoingMessage } from 'node-internal:internal_http_outgoing';
+import { IncomingMessage } from 'node-internal:internal_http_incoming';
 import { Agent, globalAgent } from 'node-internal:internal_http_agent';
 import type { IncomingMessageCallback } from 'node-internal:internal_http_util';
 import type { RequestOptions } from 'node:http';
@@ -44,6 +45,7 @@ export {
   OutgoingMessage,
   Agent,
   globalAgent,
+  IncomingMessage,
 };
 export default {
   validateHeaderName,
@@ -57,4 +59,5 @@ export default {
   OutgoingMessage,
   Agent,
   globalAgent,
+  IncomingMessage,
 };

src/node/internal/internal_http_client.ts

@@ -4,10 +4,7 @@
 // Copyright Joyent and Node contributors. All rights reserved. MIT license.
 
 import { _checkIsHttpToken as checkIsHttpToken } from 'node-internal:internal_http';
-import {
-  kHeadersSent,
-  kOutHeaders,
-} from 'node-internal:internal_http_outgoing';
+import { kOutHeaders } from 'node-internal:internal_http_outgoing';
 import { Buffer } from 'node-internal:internal_buffer';
 import { urlToHttpOptions, isURL } from 'node-internal:internal_url';
 import {
@@ -223,6 +220,9 @@ export class ClientRequest extends OutgoingMessage implements _ClientRequest {
     const headers = options.headers;
     if (!Array.isArray(headers)) {
       if (headers != null) {
+        if ('host' in headers) {
+          validateString(headers.host, 'host');
+        }
         for (const [key, value] of Object.entries(headers)) {
           this.setHeader(key, value as unknown as string);
         }
@@ -336,7 +336,8 @@ export class ClientRequest extends OutgoingMessage implements _ClientRequest {
     url.port = this.port;
     url.pathname = this.path;
 
-    this[kHeadersSent] = true;
+    // Sets headersSent to true
+    this._header = '';
 
     // Our fetch implementation has the following limitations.
     //

src/node/internal/internal_http_outgoing.ts

@@ -8,6 +8,7 @@ import {
   ERR_HTTP_HEADERS_SENT,
   ERR_INVALID_ARG_TYPE,
   ERR_STREAM_CANNOT_PIPE,
+  ERR_STREAM_DESTROYED,
   ERR_METHOD_NOT_IMPLEMENTED,
 } from 'node-internal:internal_errors';
 import {
@@ -16,13 +17,16 @@ import {
 } from 'node-internal:internal_http';
 import { Writable } from 'node-internal:streams_writable';
 import { EventEmitter } from 'node-internal:events';
-import type { OutgoingMessage as _OutgoingMessage } from 'node:http';
+import type {
+  OutgoingMessage as _OutgoingMessage,
+  OutgoingHttpHeaders,
+} from 'node:http';
 
 export const kUniqueHeaders = Symbol('kUniqueHeaders');
 export const kHighWaterMark = Symbol('kHighWaterMark');
 export const kNeedDrain = Symbol('kNeedDrain');
 export const kOutHeaders = Symbol('kOutHeaders');
-export const kHeadersSent = Symbol('kHeadersSent');
+export const kErrored = Symbol('kErrored');
 
 export function parseUniqueHeadersOption(
   headers?: string[] | null
@@ -39,11 +43,30 @@ export function parseUniqueHeadersOption(
 }
 
 export class OutgoingMessage extends Writable implements _OutgoingMessage {
-  [kHeadersSent]: boolean = false;
-  [kOutHeaders]: Record<string, Record<string, string | string[]>> = {};
+  [kOutHeaders]: Record<string, { name: string; value: string | string[] }> =
+    {};
+  [kErrored]: Error | null = null;
+
+  #header: unknown;
+
+  override writable = true;
+  override destroyed = false;
+  finished = false;
+
+  get _header(): unknown {
+    return this.#header;
+  }
+
+  set _header(value: unknown) {
+    this.#header = value;
+  }
+
+  get header(): unknown {
+    return this.#header;
+  }
 
   setHeader(name: string, value: string | string[]): this {
-    if (this[kHeadersSent]) {
+    if (this.headersSent) {
       throw new ERR_HTTP_HEADERS_SENT('set');
     }
     validateHeaderName(name);
@@ -61,7 +84,7 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
   setHeaders(
     headers: Headers | Map<string, string | number | readonly string[]>
   ): this {
-    if (this[kHeadersSent]) {
+    if (this.headersSent) {
       throw new ERR_HTTP_HEADERS_SENT('set');
     }
 
@@ -103,7 +126,7 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
   }
 
   appendHeader(name: string, value: string | string[]): this {
-    if (this[kHeadersSent]) {
+    if (this.headersSent) {
       throw new ERR_HTTP_HEADERS_SENT('append');
     }
 
@@ -118,7 +141,7 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
 
     // Prepare the field for appending, if required
     if (!Array.isArray(headers[field].value)) {
-      headers[field].value = [headers[field].value as string];
+      headers[field].value = [headers[field].value];
     }
 
     const existingValues = headers[field].value;
@@ -144,7 +167,7 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
   removeHeader(name: string): void {
     validateString(name, 'name');
 
-    if (this[kHeadersSent]) {
+    if (this.headersSent) {
       throw new ERR_HTTP_HEADERS_SENT('remove');
     }
 
@@ -165,11 +188,24 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
   }
 
   flushHeaders(): void {
-    // Not implemented
+    if (!this._header) {
+      this._implicitHeader();
+    }
+
+    // Force-flush the headers.
+    this._send('');
+  }
+
+  getHeaders(): OutgoingHttpHeaders {
+    const headers: Record<string, string | string[]> = {};
+    for (const [_key, entry] of Object.entries(this[kOutHeaders])) {
+      headers[entry.name] = entry.value;
+    }
+    return headers;
   }
 
   get headersSent(): boolean {
-    return this[kHeadersSent];
+    return this.#header != null;
   }
 
   // @ts-expect-error TS2416 Unnecessary type assertion
@@ -184,4 +220,60 @@ export class OutgoingMessage extends Writable implements _OutgoingMessage {
   _implicitHeader(): void {
     throw new ERR_METHOD_NOT_IMPLEMENTED('_implicitHeader()');
   }
+
+  _renderHeaders(): Record<string, string | string[]> {
+    if (this.headersSent) {
+      throw new ERR_HTTP_HEADERS_SENT('render');
+    }
+    const headers: Record<string, string | string[]> = {};
+    for (const [_key, entry] of Object.entries(this[kOutHeaders])) {
+      headers[entry.name] = entry.value;
+    }
+    return headers;
+  }
+
+  _send(_val: unknown): void {
+    // Unimplemented.
+  }
+
+  override _write(
+    _chunk: any, // eslint-disable-line @typescript-eslint/no-explicit-any
+    _encoding: BufferEncoding,
+    cb: (error?: Error | null) => void
+  ): void {
+    // The only reason for us to override this method is to increase the Node.js test coverage.
+    // Otherwise, we don't implement _write yet.
+    if (this.destroyed) {
+      cb(new ERR_STREAM_DESTROYED('_write'));
+      return;
+    }
+
+    throw new ERR_METHOD_NOT_IMPLEMENTED('_write');
+  }
+
+  override destroy(err?: unknown, _cb?: (err?: unknown) => void): this {
+    if (this.destroyed) {
+      return this;
+    }
+    this.destroyed = true;
+    this[kErrored] = err as Error;
+
+    return this;
+  }
+
+  // @ts-expect-error TS2611 Property accessor.
+  get writableObjectMode(): boolean {
+    return false;
+  }
+
+  // @ts-expect-error TS2611 Property accessor.
+  get errored(): Error | null {
+    return this[kErrored];
+  }
+
+  // @ts-expect-error TS2611 Property accessor.
+  get writableEnded(): boolean {
+    // eslint-disable-next-line @typescript-eslint/no-deprecated
+    return this.finished;
+  }
 }

src/workerd/api/node/tests/BUILD.bazel

@@ -325,48 +325,70 @@ wd_test(
     data = ["streams-nodejs-test.js"],
 )
 
+js_binary(
+    name = "http-nodejs-server",
+    entry_point = "http-nodejs-server.js",
+)
+
 wd_test(
     src = "http-nodejs-test.wd-test",
     args = ["--experimental"],
     data = ["http-nodejs-test.js"],
+    sidecar = "http-nodejs-server",
+    sidecar_port_bindings = [
+        "PONG_SERVER_PORT",
+        "ASD_SERVER_PORT",
+        "TIMEOUT_SERVER_PORT",
+        "HELLO_WORLD_SERVER_PORT",
+        "HEADER_VALIDATION_SERVER_PORT",
+    ],
+    tags = ["requires-network"],
+)
+
+js_binary(
+    name = "http-outgoing-nodejs-server",
+    entry_point = "http-outgoing-nodejs-server.js",
 )
 
 wd_test(
     src = "http-outgoing-nodejs-test.wd-test",
     args = ["--experimental"],
     data = ["http-outgoing-nodejs-test.js"],
+    sidecar = "http-outgoing-nodejs-server",
+    sidecar_port_bindings = [
+        "FINISH_WRITABLE_PORT",
+        "WRITABLE_FINISHED_PORT",
+        "PROPERTIES_PORT",
+    ],
 )
 
 js_binary(
-    name = "http-client-server",
-    entry_point = "http-client-server.js",
+    name = "http-client-nodejs-server",
+    entry_point = "http-client-nodejs-server.js",
 )
 
 wd_test(
     src = "http-client-nodejs-test.wd-test",
     args = ["--experimental"],
     data = ["http-client-nodejs-test.js"],
-    sidecar = "http-client-server",
+    sidecar = "http-client-nodejs-server",
     sidecar_port_bindings = [
         "PONG_SERVER_PORT",
         "ASD_SERVER_PORT",
-        "TIMEOUT_SERVER_PORT",
-        "HELLO_WORLD_SERVER_PORT",
-        "HEADER_VALIDATION_SERVER_PORT",
+        "DEFAULT_HEADERS_EXIST_PORT",
     ],
-    tags = ["requires-network"],
 )
 
 js_binary(
-    name = "http-agent-server",
-    entry_point = "http-agent-server.js",
+    name = "http-agent-nodejs-server",
+    entry_point = "http-agent-nodejs-server.js",
 )
 
 wd_test(
     src = "http-agent-nodejs-test.wd-test",
     args = ["--experimental"],
     data = ["http-agent-nodejs-test.js"],
-    sidecar = "http-agent-server",
+    sidecar = "http-agent-nodejs-server",
     sidecar_port_bindings = [
         "PONG_SERVER_PORT",
     ],

src/workerd/api/node/tests/http-agent-server.js renamed to src/workerd/api/node/tests/http-agent-nodejs-server.js

@@ -3,6 +3,7 @@
 //     https://opensource.org/licenses/Apache-2.0
 
 import http from 'node:http';
+import https from 'node:https';
 import { strictEqual, ok } from 'node:assert';
 
 export const checkPortsSetCorrectly = {
@@ -75,3 +76,54 @@ export const testHttpAgentNull = {
     await promise;
   },
 };
+
+// Test is taken from test/parallel/test-https-agent-constructor.js
+export const testHttpsAgentConstructor = {
+  async test() {
+    ok(new https.Agent() instanceof https.Agent);
+    strictEqual(typeof https.request, 'function');
+    strictEqual(typeof http.get, 'function');
+  },
+};
+
+// Tests covering http-agent
+// - [ ] test/parallel/test-http-agent-abort-controller.js
+// - [ ] test/parallel/test-http-agent-close.js
+// - [ ] test/parallel/test-http-agent-destroyed-socket.js
+// - [ ] test/parallel/test-http-agent-domain-reused-gc.js
+// - [ ] test/parallel/test-http-agent-error-on-idle.js
+// - [ ] test/parallel/test-http-agent-false.js
+// - [x] test/parallel/test-http-agent-getname.js
+// - [ ] test/parallel/test-http-agent-keepalive-delay.js
+// - [ ] test/parallel/test-http-agent-keepalive.js
+// - [ ] test/parallel/test-http-agent-maxsockets-respected.js
+// - [ ] test/parallel/test-http-agent-maxsockets.js
+// - [ ] test/parallel/test-http-agent-maxtotalsockets.js
+// - [ ] test/parallel/test-http-agent-no-protocol.js
+// - [x] test/parallel/test-http-agent-null.js
+// - [ ] test/parallel/test-http-agent-remove.js
+// - [ ] test/parallel/test-http-agent-reuse-drained-socket-only.js
+// - [ ] test/parallel/test-http-agent-scheduling.js
+// - [ ] test/parallel/test-http-agent-timeout-option.js
+// - [ ] test/parallel/test-http-agent-timeout.js
+// - [ ] test/parallel/test-http-agent-uninitialized-with-handle.js
+// - [ ] test/parallel/test-http-agent.js
+// - [ ] test/parallel/test-https-agent-abort-controller.js
+// - [ ] test/parallel/test-https-agent-additional-options.js
+// - [x] test/parallel/test-https-agent-constructor.js
+// - [ ] test/parallel/test-https-agent-create-connection.js
+// - [ ] test/parallel/test-https-agent-disable-session-reuse.js
+// - [ ] test/parallel/test-https-agent-getname.js
+// - [ ] test/parallel/test-https-agent-keylog.js
+// - [ ] test/parallel/test-https-agent-servername.js
+// - [ ] test/parallel/test-https-agent-session-eviction.js
+// - [ ] test/parallel/test-https-agent-session-injection.js
+// - [ ] test/parallel/test-https-agent-session-reuse.js
+// - [ ] test/parallel/test-https-agent-sni.js
+// - [ ] test/parallel/test-https-agent-sockets-leak.js
+// - [ ] test/parallel/test-https-agent-unref-socket.js
+// - [ ] test/parallel/test-https-agent.js
+
+// Tests doesn't make sense for workerd:
+//
+// - [ ] test/parallel/test-http-agent-uninitialized.js