Repository navigation
List of tests where console.log|error can be replaced with debug #32678
Description
This issue is to track all the test files that have console.log|error that can be replaced with debug statements. The consideration is that only informative messages can be replaced, not the one that influence the test logic itself.
A good candidate for a future code & learn. Recommend one per person, doing more does not add value.
Refs: #32588 (comment)
Checklist for reference
test/parallel
- test-cluster-setup-master-cumulative.js
- test-cluster-setup-master-multiple.js
- test-cluster-worker-wait-server-close.js
- test-code-cache.js
- test-crypto.js
- test-dgram-close.js
- test-domain-http-server.js
- test-domain-stack.js
- test-event-emitter-listeners-side-effects.js
- test-event-emitter-subclass.js
- test-exception-handler2.js
[I am still building the list]
Activity
HarshithaKP commentedon Apr 6, 2020
Can someone label this as "good-first-issue"?
gireeshpunathil commentedon Apr 6, 2020
/cc @nodejs/testing
himself65 commentedon Apr 6, 2020
I think
const debug = require('util').debuglog('test')can be extracted to the `common' module if we really do thisusing
Trott commentedon Apr 6, 2020
The
console.log()statements intest-cli-eval.jsare part of the test and changing them todebugwill make the test fail, as it should. Those statement should be left alone. That test should not be on the list, I don't think.Trott commentedon Apr 6, 2020
Same for
test-cli-node-options.js. Shouldn't be changed.Trott commentedon Apr 6, 2020
By the way, there are (at the time of this writing) still 8 open code-and-learn PRs that can be helped to a conclusion.
daemon1024 commentedon Apr 6, 2020
Can I work on this issue ?
From what I understand, I just need to replace
console.log/error(message)withdebug(message)across the listed tests.HarshithaKP commentedon Apr 6, 2020
@daemon1024, yes please; but would suggest to pick up only (any) one item.
agustindaguerre commentedon Apr 6, 2020
I would like to work on this issue as well, may I take
test-cluster-setup-master-multiple.js?daemon1024 commentedon Apr 6, 2020
Great @HarshithaKP
Sure. I will start with working on
test-domain-http-server.jsaddaleax commentedon Apr 6, 2020
Not to bring anybody down, but … can we spell out what the motivation for this is? All I can see coming out as a result of this is that some tests become harder to debug when they fail in CI.
daemon1024 commentedon Apr 6, 2020
@addaleax as mentioned in the reference comment .
Should clarify things up.
Trott commentedon Apr 6, 2020
Yeah, here's the thing. That's a problem that comes up infrequently. It happens. I've been bit by it. But the problem of a test intermittently timing out in CI and there not being enough information to figure out what's going on? Happens a lot. Way more often than
console.log()messing up a test with timing, and so on. And it often happens that the test only times out once in 100 runs or 1000 runs. And often the stress test job can't reproduce it because it only happens under certain conditions on a certain machine, but that's not obvious at the outset.I will take some extra information that confuses me a little bit once in a long while over spending hours trying to reproduce a test failure so I can debug it.
I'm not opposed to this. I'm not going to block it. But I am, sorry to say, unenthusiastic.
himself65 commentedon Apr 6, 2020
we could enable debug option when test fail and rerun it then output the error log in CI. I think it’s not a big problem.
what's more, we can make the logs usually look very clear if do this
18 remaining items
addaleax commentedon Apr 9, 2020
As an extra data point, I just ran into this with 1a3c747 – the test in question takes a long time to run, so waiting for it to finish, seeing it fail, and then having to re-run it with a special environment variable just to get the output that was present before that commit is definitely annoying.
I’d be -1 on further PRs that do this unless there’s reason to believe that they affect test functionality in a positive way.
HarshithaKP commentedon Apr 10, 2020
Closing this based on the feedback so far. My intent was definitely to improve tests, but this one does not look like helping towards that.
test: replace console.log/error with debuglog
test: replace console.log/error with debuglog
test: replace console.log/error() with debuglog
test: replace console.log/error() with debuglog
test: replace console.log/error with debuglog
test: replace console.log/error() with debuglog
test: replace console.log/error() with debuglog
test: replace console.log/error() with debuglog