Node: Add a common.log for tests

[gibfahn]

• Subsystem: Test

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

[gibfahn]

see comments:

@cjihrig in https://github.com/nodejs/node/issues/9269#issuecomment-256340473

Before we add any flag, I think we (the project) should come to some consensus on console.log() statements that are unrelated to the test outcome.

@jasnell in https://github.com/nodejs/node/pull/9264#issuecomment-256115110

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 in https://github.com/nodejs/node/pull/9264#issuecomment-256249095

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.

[gibfahn]

@jasnell in https://github.com/nodejs/node/pull/9264#issuecomment-256249095

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.

[sam-github]

Why not just encourage use of https://nodejs.org/api/util.html#util_util_debuglog_section with a section of test? Or implement common.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]

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]

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.

[gibfahn]

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

[targos]

@gibfahn would you like to pursue this?

[refack]

I would suggest monkey patching console inside on common/index.js.

• make it so that node test\X\Y prints
• python tools\test.py X/Y doesn't (necessarily) or pipes into a file

[gibfahn]

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.

@gibfahn would you like to pursue this?

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]

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.

Yep, changed my mind.

[Trott]

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.

[gibfahn]

I think it's worth leaving this open as something someone could pick up at some point.

[juggernaut451]

@gibfahn I would like to work on it. Can you please mentor on this

[palash25]

Is someone still working on this?

[chrisbautista]

Can you mentor me on this?

[apapirovski]

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.