-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: improve tty.getColorDepth coverage #19446
Conversation
Lies 😉 |
f0e6390
to
5fe0953
Compare
} | ||
|
||
process.env.NODE_DISABLE_COLORS = NODE_DISABLE_COLORS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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…
Comment addressed. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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>
Landed in ce2ed58 |
Should only be backported to Edit: wrong reference |
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>
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), orvcbuild test
(Windows) passes