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

node:test logs verbose feature #43574

Closed
mtsluna opened this issue Jun 26, 2022 · 18 comments
Closed

node:test logs verbose feature #43574

mtsluna opened this issue Jun 26, 2022 · 18 comments
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@mtsluna
Copy link

mtsluna commented Jun 26, 2022

What is the problem this feature will solve?

This feature solve the problem to turn on and turn off the logs (info/warn/error/log/etc) in the testing runner (node 18)

What is the feature you are proposing to solve the problem?

Add a simple flag in the node commands that allow to turn on the logs in combination of the next command:
node --test --verbose with the testing runner in node 18.

What alternatives have you considered?

No response

@mtsluna mtsluna added the feature request Issues that request new features to be added to Node.js. label Jun 26, 2022
@MoLow
Copy link
Member

MoLow commented Jun 26, 2022

Hi @mtsluna what type of logs do you refer to? do you have a code example to describe how such a log might look, or maybe a reference to a test runner (i.e mocha, jest,tap) that supports this?

@F3n67u F3n67u added the test_runner Issues and PRs related to the test runner subsystem. label Jun 27, 2022
@ErickWendel
Copy link
Member

ErickWendel commented Jul 13, 2022

I think this could be kind of an interesting feature. Although, I'd suggest adding a --silent to hide logs as it wouldn't have to be a breaking change. What do you think?

cc @benjamingr

@MoLow
Copy link
Member

MoLow commented Jul 13, 2022

@ErickWendel what logs would --silent mute?

@ErickWendel
Copy link
Member

@ErickWendel what logs would --silent mute?

when we run node --tests we see the node tap output right? the --silent would be used on CI and actually omit all logs so if everything is going right it shouldn't show anything and exit with code 0

@mtsluna
Copy link
Author

mtsluna commented Jul 13, 2022

Hello @MoLow and @ErickWendel when you run jest test and you put the flag --silent=false, you can see all the logs in all levels in the test console. In this case in the node:test api you cant enable the logs, you only can view the results of the execution.

@benjamingr
Copy link
Member

@nodejs/test_runner @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented Jul 13, 2022

I don't hate the idea of a quiet mode, but I do have two thoughts:

  1. If we figure out reporters, we could theoretically have a "quiet" reporter instead of this other mode of operation.
  2. Since the test runner is part of Node core, we need to balance adding functionality with not adding a ton of new CLI flags to core, and I'm not sure this is worth adding a new flag to core.

@MoLow
Copy link
Member

MoLow commented Jul 13, 2022

the --silent would be used on CI and actually omit all logs so if everything is going right it shouldn't show anything and exit with code 0

the tap output is still useful in the CI in case of a failure - users are usually interested at the specific failing test details

if we figure out reporters, we could theoretically have a "quiet" reporter instead of this other mode of operation.

+1 on that, also reporting to a file instead of stdout will be much easier and might solve this usecase

Since the test runner is part of Node core, we need to balance adding functionality with not adding a ton of new CLI flags to core, and I'm not sure this is worth adding a new flag to core.

this is probably out of scope - but we might want to introduce some kind of a .testrc file - for allowing configuration of the test runner (what reporters to use, concurrency etc), can be cleaner than adding lots of flags

@khaosdoctor
Copy link
Member

this is probably out of scope - but we might want to introduce some kind of a .testrc file - for allowing configuration of the test runner (what reporters to use, concurrency etc), can be cleaner than adding lots of flags

+1 on that, it would greatly help sharing configuration outside of the project, although I see #44241 has already been implemented


Another thought on that would be to check the NODE_ENV for whether you're running the app on development (as those are quite common) if you're running in test, production, or anything other than development then console.logs would be suppressed, otherwise, they'd be shown.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 25, 2022

I am strongly -1 to using NODE_ENV for anything in the test runner.

@MoLow
Copy link
Member

MoLow commented Aug 25, 2022

I am waiting to see how #43973 evolves to figure out how to globally configure node,
until then #44241 is a perfect way to achieve quite mode

using NODE_ENV sounds like a hack to me

@khaosdoctor
Copy link
Member

khaosdoctor commented Aug 29, 2022

I am strongly -1 to using NODE_ENV for anything in the test runner.

Any particular reason? Just curious on the drawbacks that I might not have thought 😅

@cjihrig
Copy link
Contributor

cjihrig commented Aug 29, 2022

A few reasons:

  • Despite popularity, NODE_ENV is an anti-pattern, especially when the default value is not production. I've seen multiple apps running in production without setting NODE_ENV properly, so their performance was not as good as it could/should have been.
  • This is a test runner, so I'd expect NODE_ENV to be 'test' or 'development' all the time.

@khaosdoctor
Copy link
Member

I see! Thanks for the explanation.

@himself65
Copy link
Member

I don't see any usage of NODE_ENV in the nodejs source code. Only in npm and third-party libraries.
So -1 on this, because I think nodejs runtime shouldn't rely on any environment variables, this will make users hard to debug

@himself65
Copy link
Member

Not only console.log, I'm using util.debugLog, but it also cannot display to the console or somewhere else

@Qix-
Copy link

Qix- commented Sep 14, 2022

@himself65 to be fair, debugLog already relies on NODE_DEBUG (akin to debug).

@cjihrig
Copy link
Contributor

cjihrig commented Dec 19, 2022

I'm going to close this. Support for reporters just landed in a1b27b2, so you can customize the output how you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

No branches or pull requests

10 participants