Skip to content

test: improve numerous http tests #12930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

test: improve numerous http tests #12930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions test/parallel/test-http-1.0-keep-alive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
const http = require('http');
const net = require('net');

Expand Down Expand Up @@ -107,7 +107,8 @@ function check(tests) {
const test = tests[0];
let server;
if (test) {
server = http.createServer(serverHandler).listen(0, '127.0.0.1', client);
server = http.createServer(serverHandler)
.listen(0, '127.0.0.1', common.mustCall(client));
Copy link
Contributor

Choose a reason for hiding this comment

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

/s/127.0.0.1/common.localhostIPv4/

}
let current = 0;

Expand All @@ -118,9 +119,8 @@ function check(tests) {
function serverHandler(req, res) {
if (current + 1 === test.responses.length) this.close();
const ctx = test.responses[current];
console.error('< SERVER SENDING RESPONSE', ctx);
res.writeHead(200, ctx.headers);
ctx.chunks.slice(0, -1).forEach(function(chunk) { res.write(chunk); });
ctx.chunks.slice(0, -1).forEach((chunk) => res.write(chunk));
res.end(ctx.chunks[ctx.chunks.length - 1]);
}

Expand All @@ -131,19 +131,16 @@ function check(tests) {

function connected() {
const ctx = test.requests[current];
console.error(' > CLIENT SENDING REQUEST', ctx);
conn.setEncoding('utf8');
conn.write(ctx.data);

function onclose() {
console.error(' > CLIENT CLOSE');
if (!ctx.expectClose) throw new Error('unexpected close');
client();
}
conn.on('close', onclose);

function ondata(s) {
console.error(' > CLIENT ONDATA %j %j', s.length, s.toString());
current++;
if (ctx.expectClose) return;
conn.removeListener('close', onclose);
Expand Down
17 changes: 6 additions & 11 deletions test/parallel/test-http-1.0.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,21 @@ function test(handler, request_generator, response_validator) {
let server_response = '';

server.listen(0);
server.on('listening', function() {
const c = net.createConnection(this.address().port);
server.on('listening', common.mustCall(() => {
const c = net.createConnection(server.address().port);

c.setEncoding('utf8');

c.on('connect', function() {
c.write(request_generator());
});
c.on('connect', common.mustCall(() => c.write(request_generator())));
c.on('data', common.mustCall((chunk) => server_response += chunk));
Copy link
Contributor

@cjihrig cjihrig May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should add common.mustCall() around 'data' event handlers because they can be called a number of times.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea for me is that tests should be predictable, and that for each of these tests, the number of times an event like data would be called should be deterministic. If a change is made that changes that, then the test should pick up on it and update the test. Doing so would increase the visibility of such changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

common.mustCall() takes an expected:int as second argument.
I'll write a PR to have it expect '+' as "more than zero".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with that point. But aren't we at the mercy of different platforms and whatnot for the flow of data? If it can be done reliably, and without platform specific checks, then 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell FWIW #12935

Copy link
Member

@Trott Trott May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Given that there's no way for the test to pass if the callback is not called (because server_response will not have the expected data), I would be inclined to drop common.mustCall(). The more layers of stuff added to the test, the harder it is to figure out what the test is actually testing.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Trott.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW fccc0bf landed now you have common.mustCallAtLeast


c.on('data', function(chunk) {
server_response += chunk;
});

c.on('end', common.mustCall(function() {
c.on('end', common.mustCall(() => {
client_got_eof = true;
c.end();
server.close();
response_validator(server_response, client_got_eof, false);
}));
});
}));
}

{
Expand Down
6 changes: 1 addition & 5 deletions test/parallel/test-http-abort-before-end.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ server.listen(0, function() {
port: this.address().port
});

req.on('error', function(ex) {
// https://github.com/joyent/node/issues/1399#issuecomment-2597359
// abort() should emit an Error, not the net.Socket object
assert(ex instanceof Error);
});
req.on('error', common.mustNotCall());

req.abort();
req.end();
Expand Down
36 changes: 9 additions & 27 deletions test/parallel/test-http-abort-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,42 +23,24 @@
const common = require('../common');
const http = require('http');

const server = http.Server(function(req, res) {
console.log('Server accepted request.');
const server = http.Server(common.mustCall((req, res) => {
res.writeHead(200);
res.write('Part of my res.');

res.destroy();
});
}));

server.listen(0, common.mustCall(function() {
server.listen(0, common.mustCall(() => {
http.get({
port: this.address().port,
port: server.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall(function(res) {
}, common.mustCall((res) => {
server.close();

console.log(`Got res: ${res.statusCode}`);
console.dir(res.headers);

res.on('data', function(chunk) {
console.log(`Read ${chunk.length} bytes`);
console.log(' chunk=%j', chunk.toString());
});

res.on('end', function() {
console.log('Response ended.');
});

res.on('aborted', function() {
console.log('Response aborted.');
});

res.socket.on('close', function() {
console.log('socket closed, but not res');
});

// it would be nice if this worked:
res.on('data', common.mustCall());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe replace with the longer but more explicit:

    let data = '';
    res.on('data', (chunk) => { data += chunk; });
    res.on('end', common.mustCall(() => {
      assert.strictEqual(data, 'Part of my res.');
    }));

This way it verifies all of these:

  • the data callback is called at least once
  • the data callback is sent the expected value(s)
  • the 'end callback is only fired after the data callback(s)

res.on('end', common.mustCall());
res.on('aborted', common.mustCall());
res.socket.on('close', common.mustCall());
res.on('close', common.mustCall());
}));
}));
34 changes: 12 additions & 22 deletions test/parallel/test-http-after-connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,36 +26,29 @@ const http = require('http');

let clientResponses = 0;

const server = http.createServer(common.mustCall(function(req, res) {
console.error('Server got GET request');
const server = http.createServer(common.mustCall((req, res) => {
req.resume();
res.writeHead(200);
res.write('');
setTimeout(function() {
res.end(req.url);
}, 50);
setTimeout(() => res.end(req.url), 50);
}, 2));
server.on('connect', common.mustCall(function(req, socket) {
console.error('Server got CONNECT request');
server.on('connect', common.mustCall((req, socket) => {
socket.write('HTTP/1.1 200 Connection established\r\n\r\n');
socket.resume();
socket.on('end', function() {
socket.end();
});
socket.on('end', () => socket.end());
}));
server.listen(0, function() {
server.listen(0, () => {
const req = http.request({
port: this.address().port,
port: server.address().port,
method: 'CONNECT',
path: 'google.com:80'
});
req.on('connect', common.mustCall(function(res, socket) {
console.error('Client got CONNECT response');
req.on('connect', common.mustCall((res, socket) => {
socket.end();
socket.on('end', function() {
socket.on('end', common.mustCall(() => {
doRequest(0);
doRequest(1);
});
}));
socket.resume();
}));
req.end();
Expand All @@ -65,14 +58,11 @@ function doRequest(i) {
http.get({
port: server.address().port,
path: `/request${i}`
}, common.mustCall(function(res) {
console.error('Client got GET response');
}, common.mustCall((res) => {
let data = '';
res.setEncoding('utf8');
res.on('data', function(chunk) {
data += chunk;
});
res.on('end', function() {
res.on('data', (chunk) => data += chunk);
res.on('end', () => {
assert.strictEqual(data, `/request${i}`);
++clientResponses;
if (clientResponses === 2) {
Expand Down
42 changes: 16 additions & 26 deletions test/parallel/test-http-agent-destroyed-socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,28 @@ const common = require('../common');
const assert = require('assert');
const http = require('http');

const server = http.createServer(function(req, res) {
const server = http.createServer(common.mustCall((req, res) => {
res.writeHead(200, {'Content-Type': 'text/plain'});
res.end('Hello World\n');
}).listen(0, common.mustCall(function() {
}, 2)).listen(0, common.mustCall(() => {
const agent = new http.Agent({maxSockets: 1});

agent.on('free', function(socket) {
console.log('freeing socket. destroyed? ', socket.destroyed);
});
agent.on('free', common.mustCall(3));

const requestOptions = {
agent: agent,
host: 'localhost',
port: this.address().port,
port: server.address().port,
path: '/'
};

const request1 = http.get(requestOptions, common.mustCall(function(response) {
const request1 = http.get(requestOptions, common.mustCall((response) => {
// assert request2 is queued in the agent
const key = agent.getName(requestOptions);
assert.strictEqual(agent.requests[key].length, 1);
console.log('got response1');
request1.socket.on('close', function() {
console.log('request1 socket closed');
});
response.pipe(process.stdout);
response.on('end', common.mustCall(function() {
console.log('response1 done');
request1.socket.on('close', common.mustCall());
response.on('data', common.mustCall());
response.on('end', common.mustCall(() => {
/////////////////////////////////
//
// THE IMPORTANT PART
Expand All @@ -65,11 +59,10 @@ const server = http.createServer(function(req, res) {
// is triggered.
request1.socket.destroy();

response.once('close', function() {
response.once('close', () => {
// assert request2 was removed from the queue
assert(!agent.requests[key]);
console.log("waiting for request2.onSocket's nextTick");
process.nextTick(common.mustCall(function() {
process.nextTick(common.mustCall(() => {
// assert that the same socket was not assigned to request2,
// since it was destroyed.
assert.notStrictEqual(request1.socket, request2.socket);
Expand All @@ -79,25 +72,22 @@ const server = http.createServer(function(req, res) {
}));
}));

const request2 = http.get(requestOptions, common.mustCall(function(response) {
const request2 = http.get(requestOptions, common.mustCall((response) => {
assert(!request2.socket.destroyed);
assert(request1.socket.destroyed);
// assert not reusing the same socket, since it was destroyed.
assert.notStrictEqual(request1.socket, request2.socket);
console.log('got response2');
let gotClose = false;
let gotResponseEnd = false;
request2.socket.on('close', function() {
console.log('request2 socket closed');
request2.socket.on('close', common.mustCall(() => {
gotClose = true;
done();
});
response.pipe(process.stdout);
response.on('end', function() {
console.log('response2 done');
}));
response.on('data', common.mustCall());
response.on('end', common.mustCall(() => {
gotResponseEnd = true;
done();
});
}));

function done() {
if (gotResponseEnd && gotClose)
Expand Down
14 changes: 7 additions & 7 deletions test/parallel/test-http-agent-error-on-idle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const http = require('http');
const Agent = http.Agent;

const server = http.createServer(function(req, res) {
const server = http.createServer((req, res) => {
res.end('hello world');
});

server.listen(0, function() {
server.listen(0, () => {
const agent = new Agent({
keepAlive: true,
});

const requestParams = {
host: 'localhost',
port: this.address().port,
port: server.address().port,
agent: agent,
path: '/'
};
Expand All @@ -25,8 +25,8 @@ server.listen(0, function() {
get(function(res) {
assert.strictEqual(res.statusCode, 200);
res.resume();
res.on('end', function() {
process.nextTick(function() {
res.on('end', common.mustCall(() => {
process.nextTick(() => {
const freeSockets = agent.freeSockets[socketKey];
assert.strictEqual(freeSockets.length, 1,
`expect a free socket on ${socketKey}`);
Expand All @@ -37,7 +37,7 @@ server.listen(0, function() {

get(done);
});
});
}));
});

function get(callback) {
Expand Down
20 changes: 7 additions & 13 deletions test/parallel/test-http-agent-false.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');
const http = require('http');

Expand All @@ -35,20 +35,14 @@ const opts = {
agent: false
};

let good = false;
process.on('exit', function() {
assert(good, 'expected either an "error" or "response" event');
});

// we just want an "error" (no local HTTP server on port 80) or "response"
// to happen (user happens ot have HTTP server running on port 80).
// As long as the process doesn't crash from a C++ assertion then we're good.
const fn = common.mustCall();
const req = http.request(opts);
req.on('response', function(res) {
good = true;
});
req.on('error', function(err) {
// an "error" event is ok, don't crash the process
good = true;
});

// Both the below use the same common.mustCall() instance, so exactly one
// (but not both) would be called
req.on('response', fn);
req.on('error', fn);
req.end();
Loading