doc: add guidance on console output in tests #26456
Conversation
|
The guidance suggested in this PR seems measured and balanced to me. Here's some additional context that does not need to be integrated, but feel free if you think it's useful:
|
|
(Oh, and it might be good to suggest a standard way to note when a |
|
For myself, the issue is not really about verbosity of output. It's about not having clarity if the console output is informational or part of the actual test. For message tests, for instance, a change in a console.log can break the test needlessly. I would much rather have a separate |
Maybe just clearly defining use-cases for both |
|
I'm ok with it when it makes sense. In most cases an assertion with a good error message is sufficient though. |
There was #22206 which stalled. |
I don't think that's quite enough as it would still have the issue of not being generally appliable to every test. Message and console tests would have to use a different convention. |
|
I'm ok with what is written. It might be possible to address part of @jasnell concern by including a requirement that informational console.log start with 'INFO:' and then we can provide tooling to help do that later (although if we had agreement that was enough or enough to start with its probably easy to add a method to the test framework to add the INFO: part). |
Just throwing this out as an idea, but we could lint for |
|
Just asking, but what do folks have against using |
|
Using |
|
+1 on the overall idea, but only concern here is that whether the console statement is providing actual info about the test or not. However, as @jasnell points out, we can very well use |
|
It will result in a huge amount of churn in our tests if we want it to replace non-functionality-testing uses of |
|
As far as this PR goes, I'd like to provide guidance on current practice, so while the conversation about how we wish things were is useful, and might lead somewhere, it should be possible to land guidance for what to now. The guidance can change when there is a better way. I updated the guidance to add discussion on all the points raised above (at least, I tried). Please take another look. Right now there are 538 console.log and 297 console.error statements in test/parallel. That I can't tell from grep which are APIs under test, and which are for console output of informative messages is a good point in favour of having a specific test API, though I can't honestly say I've ever had difficulty in distinguishing while reading a test. I think Use of Converting existing tests would definitely fit into the code-n-learns well. |
|
My discomfort with adding stuff to Another issue with Is this perhaps a solution in search of a problem? As @sam-github noted, it's never been difficult (in my experience, at least) to differentiate "this is debug information" vs. "this is part of the test" in Anyway, I agree that for documenting current practice, the text that was there was good. The expanded text seems OK to me too. |
|
And just to be both extra clear and extra frustrating: Despite all my long-winded comments, I'm not (at this point) opposed to switching to either |
Seems solvable. You could have
Yeah, that one's a concern I certainly share and why I slightly favor just keeping things the way they are. |
sam-github
force-pushed
the
guide-for-output-in-tests
branch
from
76694f8
to
79d5aa3
Compare
|
@jasnell You requested changes, PTAL Probably unnecessary, but: ci: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2805/ |
|
Landed in 4d89fcc |
PR-URL: nodejs#26456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
PR-URL: nodejs#26456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
PR-URL: #26456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
PR-URL: #26456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
I'd like the test writing guide to include some information on expected usage of console.log/error, to avoid having to discuss it (as much) in individual PRs.
As long as I've been writing node.js unit tests (since 0.10), I've written my tests with console.log statements (or added them while debugging), then removed them before PRing the changes. I'm not entirely sure why this is the node.js convention, or even if it is widely agreed that its the convention (I count over 200 tests with 2 or more calls to
console.log()orconsole.error()).Output is surpressed by the test runner if the test passes, so it does not increase the noise/verbosity of most test runs, but it will be included as context if a test fails.
Since its going to be in the tap output and show up in jenkins, as well as the local display, the output should not be so excessive as to overwhelm the display, but other than that, I can't see why anyone other than a test author/maintainer should have concerns over what the test outputs.
What do other @nodejs/collaborators think?
If the consensus is that console output should be avoided, I'll modify the text. No matter what the text ends up being, by documenting it, it should make PR review clearer since there will be agreement on the general policy.
cf. discussion with @jasnell and @addaleax preceeding
#26209 (comment)
Checklist