You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It's come up a couple of times so far, so it's probably time for a proper discussion on the use of console.log in tests (i.e. should tests have any console.log output, and if so, when).
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.
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.
Every time I write a new test, or repair an existing test, I add a bunch of console log statements in... and then I remove them before PRing the tests to node. Seems a shame.
Reacted by Gibson Fahnestock, Richard Lau and Pete Nykänen
I guess there's always a balance, but I'd say that for me a console.log (or I guess a common.log()) can sometimes be as helpful as a comment, depending on the situation. I agree that too many of them adds complexity.
Using the existing debuglog in a common.log() makes sense to me.
I'm unclear right now, does stdout show up in the test runner unless the test fails? Still, I think overall it would be more helpful to have a more configurable logging helper.
I would suggest monkey patching console inside on common/index.js.
What's the advantage over creating a common.log? console.log is used in several tests as part of the test. The point would be to use common.log to signify that this is debugging information.
Sorry, missed this. I'd like to get round to this at some point, but it's been requested by plenty of people, so if someone else gets to it first that's fine by me.
I would suggest monkey patching console inside on common/index.js.
What's the advantage over creating a common.log? console.log is used in several tests as part of the test. The point would be to use common.log to signify that this is debugging information.
This issue is labeled discuss but it seems to have stalled. I'm going to close it on the grounds that someone can open a PR to implement this proposal or not, but either way, the discussion seems to have run its course. Feel free to re-open if you think this should not be closed yet.
There's no movement on this, there's no conclusion this is desired and there's no real movement. I think this should be closed unless someone can put some momentum behind it.
Activity
gibfahn commentedon Oct 27, 2016
see comments:
@cjihrig in #9269 (comment)
@jasnell in #9264 (comment)
@Trott in #9264 (comment)
gibfahn commentedon Oct 27, 2016
@jasnell in #9264 (comment)
sam-github commentedon Oct 27, 2016
Why not just encourage use of https://nodejs.org/api/util.html#util_util_debuglog_section with a section of
test? Or implementcommon.log()in terms of it?Every time I write a new test, or repair an existing test, I add a bunch of console log statements in... and then I remove them before PRing the tests to node. Seems a shame.
gibfahn commentedon Oct 27, 2016
I guess there's always a balance, but I'd say that for me a console.log (or I guess a
common.log()) can sometimes be as helpful as a comment, depending on the situation. I agree that too many of them adds complexity.Using the existing debuglog in a
common.log()makes sense to me.Fishrock123 commentedon Oct 31, 2016
I'm unclear right now, does
stdoutshow up in the test runner unless the test fails? Still, I think overall it would be more helpful to have a more configurable logging helper.gibfahn commentedon Oct 31, 2016
@Fishrock123 Right now you don't get stdout or stderr if the test passes. You do get it if the test fails.
I agree that what we really need is a way of setting how much output you want.
[-]Use of console.log in tests[/-][+]Add a common.log for tests[/+]targos commentedon May 30, 2017
@gibfahn would you like to pursue this?
refack commentedon Jun 1, 2017
I would suggest monkey patching
consoleinside oncommon/index.js.node test\X\Yprintspython tools\test.py X/Ydoesn't (necessarily) or pipes into a filegibfahn commentedon Jun 9, 2017
What's the advantage over creating a
common.log?console.logis used in several tests as part of the test. The point would be to usecommon.logto signify that this is debugging information.Sorry, missed this. I'd like to get round to this at some point, but it's been requested by plenty of people, so if someone else gets to it first that's fine by me.
refack commentedon Jun 9, 2017
Yep, changed my mind.
Trott commentedon Mar 9, 2018
This issue is labeled
discussbut it seems to have stalled. I'm going to close it on the grounds that someone can open a PR to implement this proposal or not, but either way, the discussion seems to have run its course. Feel free to re-open if you think this should not be closed yet.gibfahn commentedon Mar 9, 2018
I think it's worth leaving this open as something someone could pick up at some point.
test: implement common.log
juggernaut451 commentedon Mar 17, 2018
@gibfahn I would like to work on it. Can you please mentor on this
palash25 commentedon May 25, 2018
Is someone still working on this?
chrisbautista commentedon Oct 12, 2018
Can you mentor me on this?
apapirovski commentedon Nov 2, 2018
There's no movement on this, there's no conclusion this is desired and there's no real movement. I think this should be closed unless someone can put some momentum behind it.