-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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.
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
test: refactor the code in test-http-connect #10397
Conversation
var data = firstBodyChunk.toString(); | ||
socket.on('data', function(buf) { | ||
let data = firstBodyChunk.toString(); | ||
socket.on('data', common.mustCall((buf) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid adding common.mustCall()
to the data
handler. The number of events emitted can potentially be different, and it isn't really needed for the test.
var data = firstBodyChunk.toString(); | ||
socket.on('data', function(buf) { | ||
let data = firstBodyChunk.toString(); | ||
socket.on('data', common.mustCall((buf) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here.
assert.ok(serverGotConnect); | ||
assert.ok(clientGotConnect); | ||
process.on('exit', () => { | ||
assert.strictEqual(serverGotConnect, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using common.mustCall()
, I think these variables can be eliminated completely, as can this exit
event handler.
b419178
to
f936dea
Compare
@cjihrig , just implemented your suggestions |
var clientRequestClosed = false; | ||
req.on('close', function() { | ||
let clientRequestClosed = false; | ||
req.on('close', common.mustCall(() => { | ||
clientRequestClosed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can't this be removed now? I mean the clientRequestClosed
flag.
f936dea
to
dfd7a21
Compare
@lpinca removed as suggested, please set the CI again |
assert.equal(req.url, 'google.com:443'); | ||
server.on('connect', common.mustCall((req, socket, firstBodyChunk) => { | ||
assert.strictEqual(req.method, 'CONNECT'); | ||
assert.strictEqual(req.url, 'google.com:443'); | ||
console.error('Server got CONNECT request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can also probably be removed.
|
||
req.on('connect', function(res, socket, firstBodyChunk) { | ||
req.on('connect', common.mustCall((res, socket, firstBodyChunk) => { | ||
console.error('Client got CONNECT request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables
dfd7a21
to
4dc1aae
Compare
@lpinca removed those too |
New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/5635/ |
Landed in 72a8488. |
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use common.mustCall to control the functions execution automatically * use let and const instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * remove console.error and unnecessary variables PR-URL: #10397 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test
Description of change