-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add and assert readable/writable arguments #8815
Conversation
Currently the readable and writable arguments are not specified in the req.oncomplete method. Adding and asserting that they are always true (which is always the case for TCP). This might seem unnecessary but it can't hurt to have them to pickup any breaking modifications made to ConnectionWrap::AfterConnect in the future.
assert.equal(0, status); | ||
assert.equal(client, client_); | ||
assert.equal(req, req_); | ||
assert.equal(true, readable); |
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.
We have been moving toward using assert.strictEqual()
instead of assert.equal()
.
Thanks, I'll fix that. |
assert.equal(0, status); | ||
assert.equal(client, client_); | ||
assert.equal(req, req_); | ||
assert.equal(true, readable); | ||
assert.equal(true, writable); | ||
|
||
console.log('connected'); |
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: while you're in here, would you mind removing the unnecessary console.log()
statements ?
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: while you're in here, would you mind removing the unnecessary console.log() statements ?
Sure, no problems I'll remove them.
|
We don't put |
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.
LGTM with a green-ish CI run
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.
LGTM
Currently the readable and writable arguments are not specified in the req.oncomplete method. Adding and asserting that they are always true (which is always the case for TCP). This might seem unnecessary but it can't hurt to have them to pickup any breaking modifications made to ConnectionWrap::AfterConnect in the future. PR-URL: nodejs#8815 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in: 3c5cf12 |
Currently the readable and writable arguments are not specified in the req.oncomplete method. Adding and asserting that they are always true (which is always the case for TCP). This might seem unnecessary but it can't hurt to have them to pickup any breaking modifications made to ConnectionWrap::AfterConnect in the future. PR-URL: #8815 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Currently the readable and writable arguments are not specified in the req.oncomplete method. Adding and asserting that they are always true (which is always the case for TCP). This might seem unnecessary but it can't hurt to have them to pickup any breaking modifications made to ConnectionWrap::AfterConnect in the future. PR-URL: #8815 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
Currently the readable and writable arguments are not specified in the
req.oncomplete method. Adding and asserting that they are always true
(which is always the case for TCP). This might seem unnecessary but it
can't hurt to have them to pickup any breaking modifications made to
ConnectionWrap::AfterConnect in the future.