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

test: improve tty.getColorDepth coverage by BridgeAR · Pull Request #19446 · nodejs/node · GitHub

/ node Public

test: improve tty.getColorDepth coverage - #19446

Closed
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:increase-test-coverage
ConversationCommits (2)ChecksFiles changed
Closed

test: improve tty.getColorDepth coverage#19446
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:increase-test-coverage

Conversation

BridgeAR commented Mar 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

The tty.getColorDepth function has only a very basic coverage. This almost fully covers that function (there are some parts that can not really be tested).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem. labels Mar 19, 2018

BridgeAR commented Mar 19, 2018

Copy link
Copy Markdown
Member Author

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

lpinca commented Mar 19, 2018

Copy link
Copy Markdown
Member
  • make -j4 test (UNIX), or vcbuild test (Windows) passes

Lies 😉

BridgeAR force-pushed the increase-test-coverage branch from f0e6390 to 5fe0953 Compare March 20, 2018 00:32

BridgeAR commented Mar 20, 2018

Copy link
Copy Markdown
Member Author

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

lpinca added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 20, 2018
addaleax reviewed Mar 23, 2018
View reviewed changes
Comment thread test/pseudo-tty/test-tty-get-color-depth.js Outdated
Object.defineProperty(process, 'platform', platform);
}

process.env.NODE_DISABLE_COLORS = NODE_DISABLE_COLORS;

addaleax Mar 23, 2018

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 test fails when being run with NODE_PENDING_DEPRECATION=1, because NODE_DISABLE_COLORS can be undefined at this point.

I guess you could just drop this line? Setting an environment variable at the end of the script shouldn’t make a difference anyway, I think…

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 23, 2018

BridgeAR commented Mar 24, 2018

Copy link
Copy Markdown
Member Author

Comment addressed. PTAL.

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

mcollina approved these changes Mar 26, 2018
View reviewed changes

mcollina left a comment

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

LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 27, 2018
test: improve tty.getColorDepth coverage
ce2ed58 
PR-URL: nodejs#19446
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>

BridgeAR commented Mar 27, 2018

Copy link
Copy Markdown
Member Author

Landed in ce2ed58

BridgeAR closed this Mar 27, 2018

targos commented Apr 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Should only be backported to v9.x-staging if #18800 is.

Edit: wrong reference

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018
test: improve tty.getColorDepth coverage
c6fe531 
PR-URL: nodejs#19446
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
BridgeAR deleted the increase-test-coverage branch April 1, 2019 23:39
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

addaleax addaleax left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
+1 more reviewer
Leko Leko approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL