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

test: replace console.error() with debuglog calls #32588

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 31, 2020

Somehow thought I did this in 8905be2
but clearly did not.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 31, 2020
Trott referenced this pull request Mar 31, 2020
Replace magic numbers with constant.
Use for loop for repeated code.
Reduce console logging.
Remove unneeded countdown module use.

PR-URL: #32547
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 31, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

@HarshithaKP
Copy link
Member

@Trott, there are a 100 places where console.error is present. Do you recommend we should replace this with debug statements?

@Trott
Copy link
Member Author

Trott commented Apr 1, 2020

@Trott, there are a 100 places where console.error is present. Do you recommend we should replace this with debug statements?

I'd prefer not until we have a clear/obvious/documented way to cause those debug statements to show up in CI (Jenkins and GitHub Actions).

We'd also have to be careful not to do this in tests where console.error() itself is being tested.

I'm doing this one because I felt bad because I somehow messed it up when I just landed a change to the test a day ago.

Other people (notably @jasnell) might be considerably more enthusiastic about it than I am, though.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2020

@HarshithaKP :

there are a 100 places where console.error is present. Do you recommend we should replace this with debug statements?

Incrementally where it makes sense, yes. The idea is to separate output that is considered part of the test itself from informational/debug output. The rationale is to make the intent of the output clear and to avoid cases where the debug output itself may alter the outcome of the test (this is extremely rare but we have encountered it before so the probability is definitely non-zero).

What I would not do is make these changes all at once. If someone is updating a test file for another reason, then it makes sense to also make these changes. Or, if we want to document which ones can be changed, it would make a good first contribution for a new contributor to change one test at a time.

@Trott

...until we have a clear/obvious/documented way to cause those debug statements to show up in CI (Jenkins and GitHub Actions).

@nodejs/build can enable that by optionally adding NODE_DEBUG=test to the environment of CI jobs. Note, however, that this output has rarely been helpful in diagnosing issues reported in CI, and when a test fails, it is trivial for someone investigating the issue to enable NODE_DEBUG=test in their local environment, so I would not consider this to be a blocking concern.

@Trott
Copy link
Member Author

Trott commented Apr 2, 2020

@nodejs/build can enable that by optionally adding NODE_DEBUG=test to the environment of CI jobs.

Counterpoints:

  1. We shouldn't have to involve Build WG to make that happen. (Counterpoint to my counterpoint: There are other ways to do this already. They just need to be documented and/or made a bit more obvious.)
  2. That covers Jenkins only, but not GitHub Actions. (Counterpoint to my counterpoint: Jenkins is the more important one, at least at this time.)

it is trivial for someone investigating the issue to enable NODE_DEBUG=test in their local environment,

Yeah, true, that works when you can make the test fail reliably in the local environment. However, that is not the case I am concerned about. I am concerned about relatively infrequent failures ("flaky" tests) that happen on platforms that maybe the person debugging doesn't have easy access to. (This is a situation I have found myself in more often than I like, and I know I'm not the only one.)

so I would not consider this to be a blocking concern.

I would, but I don't expect everyone to agree with me on that, so it's 👍 that you don't. In any event, it's definitely a solvable problem. Someone just needs to spend a little time doing it. (On my list!)

@rvagg
Copy link
Member

rvagg commented Apr 2, 2020

you could enable it via the test-ci Makefile target, then you wouldn't need to involve Build.

@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

@Trott Technically, this is ready to land … if you think we should, feel free to go ahead.

(As you know, I don’t personally have a strong opinion here.)

@Trott
Copy link
Member Author

Trott commented Apr 3, 2020

you could enable it via the test-ci Makefile target, then you wouldn't need to involve Build.

@rvagg I hadn't considered that viable because if we do it that way, it will be on all the time in CI rather than only when requested. Considering it more now, I suppose that could be considered desirable, although it could also be a reason to not do these kinds of changes at all.
¯\(ツ)/¯ I had imagined it as an option one turns on when needed only.

@rvagg
Copy link
Member

rvagg commented Apr 3, 2020

Well, I'm not super keen on more verbose logs having to be stored by Jenkins and parsed by users, they're pretty big as they are. I'm just saying this is an option if it's needed, I don't consider myself in a position to judge whether it is or not!

Somehow thought I did this in 8905be2
but clearly did not.

PR-URL: nodejs#32588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 3, 2020

Landed in 83ebd77

@Trott Trott merged commit 83ebd77 into nodejs:master Apr 3, 2020
@Trott Trott deleted the console-error-error branch April 3, 2020 09:43
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Somehow thought I did this in 8905be2
but clearly did not.

PR-URL: #32588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
Somehow thought I did this in 8905be2
but clearly did not.

PR-URL: #32588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
Somehow thought I did this in 8905be2
but clearly did not.

PR-URL: #32588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants