Skip to content
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: remove err timer from test-http-set-timeout #9264

Closed

Conversation

BethGriggs
Copy link
Member

Checklist
  • commit message follows commit guidelines
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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 24, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2016

Thanks for opening this PR. While you're here, there are some other changes that could be made to improve the test:

  • Replace var with const where possible.
  • Remove the console statements.
  • Add common.mustCall() around functions that should be called.
  • Maybe replace x and s with a meaningful variable name.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 24, 2016

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.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Oct 24, 2016
@Trott
Copy link
Member

Trott commented Oct 25, 2016

LGTM if CI is ✅

@gibfahn
Copy link
Member

gibfahn commented Oct 25, 2016

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?

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
assert.ok(s instanceof net.Socket);
req.connection.on('timeout', function() {
console.log('got request. setting 500ms timeout');
var socket = req.connection.setTimeout(500);
Copy link
Member

Choose a reason for hiding this comment

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

Can these vars be const as well? It seems a little weird to make them constant, but if they don't change...

@jasnell
Copy link
Member

jasnell commented Oct 25, 2016

Generally console.log() output in tests should be avoided and I've removed them when I've found them. If the console.log() uses are intentional as part of the test, then a comment indicating such is helpful. The main reason I personally do not like having them there is that the extra code tends to obscure the actual test.

@Trott
Copy link
Member

Trott commented Oct 26, 2016

Generally console.log() output in tests should be avoided and I've removed them when I've found them. If the console.log() uses are intentional as part of the test, then a comment indicating such is helpful. The main reason I personally do not like having them there is that the extra code tends to obscure the actual test.

I suspect I'm in the minority on @nodejs/testing about this but:

I often find console.log() (or console.error()) messages useful. It helps to indicate why a test fails when it fails in unexpected ways. Ideally, that should be in an assert message, but that's not always possible. Also: if the failure was unexpected, there may not be an assert. (Like, if a test fails by just timing out, having a few console.log() messages to indicate where the test is hanging is useful. This is especially true if it is happening only on a particular platform in CI and someone doesn't have access to that platform locally.)

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 blah blah buffering blah blah unbuffered blah blah flush blah blah few extra milliseconds blah blah makes the test succeed when it should fail. That's a Very Bad Thing and can be frustrating.

So, all in all... ¯_(ツ)_/¯

I don't complain when other people remove (or add) console.log() statements in a test, and I mostly avoid them myself. But I do find them useful sometimes and I generally don't ask people to remove them unless they are excessive.

@Trott
Copy link
Member

Trott commented Oct 26, 2016

@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

@Trott @gibfahn ... perhaps there's a middle of the road approach we can take with regards to comments. A new common.log(...) method could be introduced that selectively generates output based on an argument or env var passed to the test runner. Default would be for the switch to be off, limiting extraneous output. Options for switching it on could include: never, always, and only-on-failure, meaning that the output would only be written out to the console if the test actually fails.

The use of common.log() within the test would be a more explicit indicator that the statement is not part of the actual test.

@Trott
Copy link
Member

Trott commented Oct 26, 2016

@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.)

@Trott
Copy link
Member

Trott commented Oct 26, 2016

CI is good except for a build failure on a Raspberry Pi which is obviously unrelated. :shipit:

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Oct 26, 2016
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

Landed in 611c50b

@jasnell jasnell closed this Oct 26, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Nov 22, 2016
@BethGriggs BethGriggs deleted the issue-9256-http-set-timeout branch February 21, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants