Skip to content

Commit

Permalink
http2: cleanup of h2 compat layer, add tests
Browse files Browse the repository at this point in the history
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: nodejs#15313
Refs: expressjs/express#3390
PR-URL: nodejs#15254
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
apapirovski authored and addaleax committed Sep 13, 2017
1 parent 855aea7 commit 59c76a6
Show file tree
Hide file tree
Showing 8 changed files with 158 additions and 138 deletions.
238 changes: 111 additions & 127 deletions lib/internal/http2/compat.js

Large diffs are not rendered by default.

7 changes: 1 addition & 6 deletions test/parallel/test-http2-compat-serverrequest-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']);

Expand All @@ -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();
Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-http2-compat-serverrequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-compat-serverresponse-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 6 additions & 1 deletion test/parallel/test-http2-compat-serverresponse-finished.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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];
Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-writehead.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-createwritereq.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 59c76a6

Please sign in to comment.