test: improve http-client coverage #33633
Conversation
| assert.throws(() => { | ||
| new ClientRequest({ insecureHTTPParser: 'wrongValue' }); | ||
| }, { | ||
| code: /ERR_INVALID_ARG_TYPE/ |
There was a problem hiding this comment.
| code: /ERR_INVALID_ARG_TYPE/ | |
| code: 'ERR_INVALID_ARG_TYPE', | |
| message: /insecureHTTPParser/ |
| const ClientRequest = require('http').ClientRequest; | ||
|
|
||
| { | ||
| const req = new ClientRequest(() => {}); |
There was a problem hiding this comment.
The test should definitely hit the mentioned code line but it would be great to write a test that actually verifies that the callback is also called and ideally, this test case would not cause any errors, since it's a legit code path.
There was a problem hiding this comment.
@BridgeAR Here is a rewrote version, which can make cb be called, but it's a little longer than previous version, so I am not sure whether it's ok, could you help me to check?
'use strict';
const common = require('../common');
const http = require('http');
const assert = require('assert');
const ClientRequest = require('http').ClientRequest;
{
const server = http.createServer(common.mustCall((req, res) => {
res.writeHead(200);
res.end('hello world');
})).listen(80, '127.0.0.1');
const req = new ClientRequest(common.mustCall((response) => {
let body = '';
response.setEncoding('utf8');
response.on('data', (chunk) => {
body += chunk;
});
response.on('end', common.mustCall(() => {
assert.strictEqual(body, 'hello world');
server.close();
}));
}));
req.end();
}|
@KuthorX thank you very much for your contribution! This is already looking pretty good, just left two minor comments. |
8ae28ff to
2935f72
Compare
|
Could someone help me to review lastest commit? Any suggestion will be appreciated. |
af3315a to
d64869d
Compare
|
Ping @nodejs/http for reviews. |
d64869d to
289444e
Compare
|
The changes look fine to me. There is only one test failing as you're using port 80. Can you use another port? |
thanks, has changed to 8080 |
|
looks like some ci fail, maybe I should merge main branch? @ShogunPanda |
|
Landed in c925039 |
PR-URL: nodejs#33633 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: James M Snell <jasnell@gmail.com>
|
After 3 years, it merged, thank you guys 😌 |
PR-URL: #33633 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33633 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: James M Snell <jasnell@gmail.com>
Based on https://coverage.nodejs.org/coverage-d12d5ef3ef8e5d9f/lib/_http_client.js.html, I try to improve coverage to line 120 (http-client-input-function), 194-197 (http-client-insecure-http-parser-error)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes