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.

Sign up for GitHub

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

Jump to bottom

test: remove deprecated error logging #3079

Closed
wants to merge 1 commit into from
Closed

test: remove deprecated error logging #3079

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 26, 2015

common.error() is just util.error() which is deprecated. Update instances to console.error().

@Trott Trott added the test Issues and PRs related to the tests. label Sep 26, 2015
@Trott
Copy link
Member Author

Trott commented Sep 27, 2015

CI: https://ci.nodejs.org/job/node-test-commit/682/

@evanlucas
Copy link
Contributor

Don't we try to not use logging in tests at all? So should we just remove these statements totally vs replacing?

@Trott
Copy link
Member Author

Trott commented Sep 28, 2015

@evanlucas I know there are some folks who definitely think the tests should not have any console.error() statements. But I don't know if that viewpoint is consensus or not. Is it?

I thought about just removing them, but I was afraid it would be a slippery slope. If I remove all these statements, why not remove all the console.error() statements in every test? But then we're talking about a very large set of changes, and I definitely favor narrower change sets.

@Trott
Copy link
Member Author

Trott commented Oct 6, 2015

Bump: This looks good or the statements should just be removed altogether?

@evanlucas
Copy link
Contributor

I would vote for just removing them. Not sure if other agree though

@Trott
Copy link
Member Author

Trott commented Oct 6, 2015

I started removing the calls to see if there were any issues. It helped find some dead code, actually. Sold. I removed them all and a few other nearby console logging calls. How's it look?

@Trott Trott changed the title test: update deprecated error logging test: remove deprecated error logging Oct 6, 2015
@Trott
test: remove deprecated error logging
80d5cd1 
common.error() is just deprecated util.error() renamed.
Remove calls to it and some other extraneous console logging
in tests.
@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

One last CI for good measure before landing: https://ci.nodejs.org/job/node-test-pull-request/444/

Trott added a commit that referenced this pull request Oct 7, 2015
@Trott
test: remove deprecated error logging
099cfbc 
common.error() is just deprecated util.error() renamed.
Remove calls to it and some other extraneous console logging
in tests.

PR-URL: #3079
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

Landed in 099cfbc.

@Trott Trott closed this Oct 7, 2015
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
29 tasks
Trott added a commit that referenced this pull request Oct 8, 2015
@Trott @jasnell
test: remove deprecated error logging
e922716 
common.error() is just deprecated util.error() renamed.
Remove calls to it and some other extraneous console logging
in tests.

PR-URL: #3079
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
@Trott Trott deleted the common-error branch January 13, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @evanlucas @bnoordhuis @jasnell