http2: cleanup of h2 compat layer, add tests

Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: #15313 Refs: expressjs/express#3390 PR-URL: #15254 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Sep 20, 2017 · 9490be3 · 9490be3
1 parent 950f7f5
commit 9490be3
Show file tree

Hide file tree

Showing 8 changed files with 158 additions and 138 deletions.
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
diff --git a/test/parallel/test-http2-compat-serverrequest-headers.js b/test/parallel/test-http2-compat-serverrequest-headers.js
@@ -21,9 +21,9 @@ server.listen(0, common.mustCall(function() {
       'foo-bar': 'abc123'
     };
 
+    assert.strictEqual(request.path, undefined);
     assert.strictEqual(request.method, expected[':method']);
     assert.strictEqual(request.scheme, expected[':scheme']);
-    assert.strictEqual(request.path, expected[':path']);
     assert.strictEqual(request.url, expected[':path']);
     assert.strictEqual(request.authority, expected[':authority']);
 
@@ -41,11 +41,6 @@ server.listen(0, common.mustCall(function() {
 
     request.url = '/one';
     assert.strictEqual(request.url, '/one');
-    assert.strictEqual(request.path, '/one');
-
-    request.path = '/two';
-    assert.strictEqual(request.url, '/two');
-    assert.strictEqual(request.path, '/two');
 
     response.on('finish', common.mustCall(function() {
       server.close();

diff --git a/test/parallel/test-http2-compat-serverrequest.js b/test/parallel/test-http2-compat-serverrequest.js
@@ -15,7 +15,6 @@ server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     const expected = {
-      statusCode: null,
       version: '2.0',
       httpVersionMajor: 2,
       httpVersionMinor: 0
@@ -24,7 +23,6 @@ server.listen(0, common.mustCall(function() {
     assert.strictEqual(request.closed, false);
     assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);
 
-    assert.strictEqual(request.statusCode, expected.statusCode);
     assert.strictEqual(request.httpVersion, expected.version);
     assert.strictEqual(request.httpVersionMajor, expected.httpVersionMajor);
     assert.strictEqual(request.httpVersionMinor, expected.httpVersionMinor);

diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js
@@ -26,7 +26,7 @@ const server = http2.createServer(common.mustCall((req, res) => {
     assert.strictEqual(res.closed, true);
   }));
 
-  if (req.path !== '/') {
+  if (req.url !== '/') {
     nextError = errors.shift();
   }
   res.destroy(nextError);

diff --git a/test/parallel/test-http2-compat-serverresponse-finished.js b/test/parallel/test-http2-compat-serverresponse-finished.js
@@ -8,13 +8,18 @@ const assert = require('assert');
 const h2 = require('http2');
 
 // Http2ServerResponse.finished
-
 const server = h2.createServer();
 server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     response.on('finish', common.mustCall(function() {
+      assert.ok(request.stream !== undefined);
+      assert.ok(response.stream !== undefined);
       server.close();
+      process.nextTick(common.mustCall(() => {
+        assert.strictEqual(request.stream, undefined);
+        assert.strictEqual(response.stream, undefined);
+      }));
     }));
     assert.strictEqual(response.finished, false);
     response.end();

diff --git a/test/parallel/test-http2-compat-serverresponse-headers.js b/test/parallel/test-http2-compat-serverresponse-headers.js
@@ -42,6 +42,31 @@ server.listen(0, common.mustCall(function() {
     response.removeHeader(denormalised);
     assert.strictEqual(response.hasHeader(denormalised), false);
 
+    common.expectsError(
+      () => response.hasHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+    common.expectsError(
+      () => response.getHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+    common.expectsError(
+      () => response.removeHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+
     [
       ':status',
       ':method',
@@ -70,6 +95,14 @@ server.listen(0, common.mustCall(function() {
       type: TypeError,
       message: 'Value must not be undefined or null'
     }));
+    common.expectsError(
+      () => response.setHeader(), // header name undefined
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
 
     response.setHeader(real, expectedValue);
     const expectedHeaderNames = [real];

diff --git a/test/parallel/test-http2-compat-serverresponse-writehead.js b/test/parallel/test-http2-compat-serverresponse-writehead.js
@@ -22,6 +22,11 @@ server.listen(0, common.mustCall(function() {
 
     response.on('finish', common.mustCall(function() {
       server.close();
+      process.nextTick(common.mustCall(() => {
+        common.expectsError(() => { response.writeHead(300); }, {
+          code: 'ERR_HTTP2_STREAM_CLOSED'
+        });
+      }));
     }));
     response.end();
   }));

diff --git a/test/parallel/test-http2-createwritereq.js b/test/parallel/test-http2-createwritereq.js
@@ -30,7 +30,7 @@ const testsToRun = Object.keys(encodings).length;
 let testsFinished = 0;
 
 const server = http2.createServer(common.mustCall((req, res) => {
-  const testEncoding = encodings[req.path.slice(1)];
+  const testEncoding = encodings[req.url.slice(1)];
 
   req.on('data', common.mustCall((chunk) => assert.ok(
     Buffer.from(testString, testEncoding).equals(chunk)