test: remove err timer from test-http-set-timeout #9264
Conversation
|
Thanks for opening this PR. While you're here, there are some other changes that could be made to improve the test:
|
|
Hm, it looks like this is a timeout test. As much as I don't like timers in tests, it might be useful in a test of this nature. EDIT: I just looked at the issue that this PR stems from. It is probably safe to remove this after all. Sorry for the noise. |
|
LGTM if CI is ✅ |
|
What's the general rule for console.logs in tests? I find them often quite helpful, especially as you only see them when a test fails. Are they discouraged? |
df42316 to
d31f3a9
Compare
Removed the errorTimer from test-http-set-timeout.js, as this timer is not necessary to test the setTimeout functionality. Also edited the console.log message on line 8 to log the correct timeout duration. Changed var to const, and added common.mustCall() to on timeout and on error callbacks. Fixes: nodejs#9256
d31f3a9 to
5045982
Compare
| assert.ok(s instanceof net.Socket); | ||
| req.connection.on('timeout', function() { | ||
| console.log('got request. setting 500ms timeout'); | ||
| var socket = req.connection.setTimeout(500); |
There was a problem hiding this comment.
Can these vars be const as well? It seems a little weird to make them constant, but if they don't change...
|
Generally |
I suspect I'm in the minority on @nodejs/testing about this but: I often find Arguing against myself, I've also seen tests that fail without log statements and then start to pass once you add the log statements. So that's an argument against the log statements. Something about So, all in all... ¯_(ツ)_/¯ I don't complain when other people remove (or add) |
|
@Trott @gibfahn ... perhaps there's a middle of the road approach we can take with regards to comments. A new The use of |
|
@jasnell I've considered that. It has its appeal. I'm not sure the additional complexity/magic in tests and resulting churn in the test dir is worth it. (Might be! I'm honestly not sure.) |
|
CI is good except for a build failure on a Raspberry Pi which is obviously unrelated. |
Removed the errorTimer from test-http-set-timeout.js, as this timer is not necessary to test the setTimeout functionality. Also edited the console.log message on line 8 to log the correct timeout duration. Changed var to const, and added common.mustCall() to on timeout and on error callbacks. Fixes: #9256 PR-URL: #9264 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 611c50b |
Removed the errorTimer from test-http-set-timeout.js, as this timer is not necessary to test the setTimeout functionality. Also edited the console.log message on line 8 to log the correct timeout duration. Changed var to const, and added common.mustCall() to on timeout and on error callbacks. Fixes: #9256 PR-URL: #9264 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Removed the errorTimer from test-http-set-timeout.js, as this timer is not necessary to test the setTimeout functionality. Also edited the console.log message on line 8 to log the correct timeout duration. Changed var to const, and added common.mustCall() to on timeout and on error callbacks. Fixes: #9256 PR-URL: #9264 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Removed the errorTimer from test-http-set-timeout.js, as this timer is not necessary to test the setTimeout functionality. Also edited the console.log message on line 8 to log the correct timeout duration. Changed var to const, and added common.mustCall() to on timeout and on error callbacks. Fixes: #9256 PR-URL: #9264 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
Affected core subsystem(s)
test
Description of change
Removed the errorTimer from test-http-set-timeout.js, as this timer is
not necessary to test the setTimeout functionality.
Also edited the console.log message on line 8 to log the correct
timeout duration.
Fixes: #9256