Skip to content

test: replace console.log/error() with debuglog #32692

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

test: replace console.log/error() with debuglog #32692

wants to merge 1 commit into from

Conversation

daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 6, 2020 •
edited
Loading

Use utility debug logs instead of console logs in in test-domain-http-server.js

Fixes: #32678

Checklist

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 6, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30603/

@himself65 himself65 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30608/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30610/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30740/

@daemon1024
Copy link
Contributor Author

Should I fix something for the failing checks?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30751/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30753/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30754/

@puzpuzpuz
Copy link
Member

Should I fix something for the failing checks?

I don't think you can, as they're not related with your changes. I've run CI build multiple times and, as the result, created #32863 to track a flaky test.

@addaleax could you advice how to proceed with this PR, considering that one of tests is constantly failing (yet, it's not related with the PR)? See https://ci.nodejs.org/job/node-test-pull-request/30753/ as an example of build result.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30770/

@addaleax
Copy link
Member

@puzpuzpuz If a test is failing often enough for PRs to become unlandable, we should mark it as flaky, so that we only get a yellow instead of a red CI, which allows PRs to land. Thanks for opening an issue, btw!

@puzpuzpuz
Copy link
Member

@addaleax Looks like we finally have a green build here. But I've noticed your comment on the original issue (#32678 (comment)) and the issue is closed as the consensus was not to go any further with swapping console.log with debug messages in tests.

This PR has enough approvals and a green build. But considering objections raised on the original issue, I'm not certain on how we're going to proceed with this PR. WDYT?

@addaleax
Copy link
Member

@puzpuzpuz I personally think that this kind of PR is not improving the debugging experience for tests, and I also personally think that blocking this PR isn’t really worth it. You can feel free to go ahead and land this, if you like 👍

puzpuzpuz pushed a commit that referenced this pull request Apr 16, 2020
@daemon1024 @puzpuzpuz
test: replace console.log/error() with debuglog
a9da656 
PR-URL: #32692
Fixes: #32678
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@puzpuzpuz
Copy link
Member

Landed in a9da656

Congrats on your first contribution! 🎉

@puzpuzpuz puzpuzpuz closed this Apr 16, 2020
@daemon1024 daemon1024 deleted the debug-test branch April 17, 2020 05:38
MylesBorins pushed a commit that referenced this pull request Apr 17, 2020
@daemon1024 @MylesBorins
test: replace console.log/error() with debuglog
a09bf3a 
PR-URL: #32692
Fixes: #32678
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@daemon1024 @targos
test: replace console.log/error() with debuglog
a728a3f 
PR-URL: nodejs#32692
Fixes: nodejs#32678
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@daemon1024 @BridgeAR
test: replace console.log/error() with debuglog
4baf41f 
PR-URL: #32692
Fixes: #32678
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
Merged
targos pushed a commit that referenced this pull request Apr 28, 2020
@daemon1024 @targos
test: replace console.log/error() with debuglog
68e3954 
PR-URL: #32692
Fixes: #32678
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request May 2, 2020
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@awibox awibox awibox approved these changes

@cjihrig cjihrig cjihrig approved these changes

@himself65 himself65 himself65 approved these changes

@trivikr trivikr trivikr approved these changes

@MrHeer MrHeer MrHeer approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

 List of tests where console.log|error can be replaced with debug
10 participants
@daemon1024 @nodejs-github-bot @puzpuzpuz @addaleax @jasnell @awibox @cjihrig @himself65 @trivikr @MrHeer