FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add tests for console.[info|error|warn] by bengl · Pull Request #6538 · nodejs/node · GitHub

/ node Public

test: add tests for console.[info|error|warn] - #6538

Closed
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:consoletests
ConversationCommits (1)ChecksFiles changed
Closed

test: add tests for console.[info|error|warn]#6538
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:consoletests

Conversation

bengl commented May 3, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Add tests for console.[info|error|warn].

Just copied the basic tests for log, as they're all the same thing
as log in either stdout or stderr.

nodejs-github-bot added the test Issues and PRs related to the tests. label May 3, 2016
evanlucas reviewed May 3, 2016
View reviewed changes
Comment thread test/parallel/test-console.js Outdated

evanlucas May 3, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

const

mscdex added the console Issues and PRs related to the console subsystem. label May 3, 2016

evanlucas commented May 3, 2016

Copy link
Copy Markdown
Contributor

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

cjihrig commented May 3, 2016

Copy link
Copy Markdown
Contributor

LGTM. CI is green.

jasnell reviewed May 3, 2016
View reviewed changes
Comment thread test/parallel/test-console.js Outdated

jasnell May 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This section is a bit of a mess in that it's not very readable and mixes the strings and errStrings checks up. This can likely be simplified by creating a secondary array with the expected strings in the right order then just doing a for-loop through each of strings and errStrings to check each one. The code would be much easier to read and there'd be much less of it :-)

bengl May 3, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, the first bunch of these can work that way since they're just testing equality. I'll do that.

For the ones doing indexOf, regex or other more complex tests, I think they're going to have to be on their own still. Some comments might be able to clarify them though.

evanlucas commented May 3, 2016

Copy link
Copy Markdown
Contributor

LGTM. CI again: https://ci.nodejs.org/job/node-test-pull-request/2487/

evanlucas commented May 3, 2016

Copy link
Copy Markdown
Contributor

hmmm looks like linting failed?

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member

LGTM

test: add tests for console.[info|error|warn]
e68449d 
Just copied the basic tests for log, as they're all the same thing
as log in either stdout or stderr. Cleaned that up a bit.

Also const-ified.

bengl commented May 4, 2016

Copy link
Copy Markdown
Member Author

@evanlucas i fixed the lint

evanlucas commented May 4, 2016

Copy link
Copy Markdown
Contributor

Thanks....let's try the CI one more time https://ci.nodejs.org/job/node-test-pull-request/2501/

bengl commented May 6, 2016

Copy link
Copy Markdown
Member Author

@evanlucas looks like a single failure, for unrelated jenkinsey reasons https://ci.nodejs.org/job/node-test-commit-arm/nodes=armv8-ubuntu1404/3137/console

evanlucas commented May 6, 2016

Copy link
Copy Markdown
Contributor

Last CI looks good (https://ci.nodejs.org/job/node-test-pull-request/2520/). Landing now. Thanks!

evanlucas pushed a commit that referenced this pull request May 6, 2016
test: add tests for console.[info|error|warn]
fd49556 
Just copied the basic tests for log, as they're all the same thing
as log in either stdout or stderr. Cleaned that up a bit.

Also const-ified.

PR-URL: #6538
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas commented May 6, 2016

Copy link
Copy Markdown
Contributor

Landed in fd49556. Thanks @bengl!

evanlucas closed this May 6, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
test: add tests for console.[info|error|warn]
8141c2f 
Just copied the basic tests for log, as they're all the same thing
as log in either stdout or stderr. Cleaned that up a bit.

Also const-ified.

PR-URL: #6538
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas mentioned this pull request May 17, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

console Issues and PRs related to the console subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL