test: actually test tty getColorDepth() #18800
Conversation
There was a problem hiding this comment.
LGTM with two suggestions. Thanks for fixing this.
There was a problem hiding this comment.
I would like that we keep using strict mode. It is something that I would like to switch over on the long term.
There was a problem hiding this comment.
@BridgeAR That lint rule does not seem to be correctly applied to /pseudo-tty/, so the linter complains.
There was a problem hiding this comment.
You will have to deactivate that rule. The rule should actually be updated. I will open an issue so it can be grabbed by someone.
test/common/index.js
Outdated
There was a problem hiding this comment.
Something similar wa sin here at some point and is used in another test file. It would be good to consolidate in that case.
There was a problem hiding this comment.
If there is, I'm not sure where it is at. If there are tests somewhere that I missed that use something like this, they probably don't actually work...? I couldn't find any in a quick look.
There are some tests that don't even bother to go for something like this and just go for fd 0 instead. (Which could probably be added to the lookup list here? I'll do that.)
There was a problem hiding this comment.
There is one in sequential/test-async-wrap-getasyncid.js. And zero was removed because it can cause issues on Windows.
There was a problem hiding this comment.
That part of that test definitely never ever runs. 😬
There was a problem hiding this comment.
I'll move that out next and base it off of this then, I suppose.
test/common/index.js
Outdated
There was a problem hiding this comment.
Since we're already importing all of fs on the immediately prior line, is this really necessary? Can we just use fs.openSync in the one place it gets used later on (line 814)?
test/common/index.js
Outdated
There was a problem hiding this comment.
This eslint-disable comment is unnecessary. Please remove it.
There was a problem hiding this comment.
Skipping TTY
tests there due to missing tty fds just means they will never be run,
ever, on any system.
/dev/tty should in practice be a pretty reliable way to get a TTY, at least for developer machines.
test/common/index.js
Outdated
There was a problem hiding this comment.
0 is also a very good candidate for this
There was a problem hiding this comment.
Interesting. I'll have to leave a note about this.
That's only true if a terminal is directly attached to the process. If I comment out these lines on node/test/parallel/test-tty-get-color-depth.js Lines 28 to 29 in 3e1e450 |
667ced5 to
f4b997d
Compare
|
Updated. Actual new CI: https://ci.nodejs.org/job/node-test-pull-request/13214/ |
f4b997d to
e745f29
Compare
|
@Fishrock123 this needs a rebase |
e745f29 to
c8bdcf8
Compare
This utility is fairly generic and likely useful for more than one test. PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
c8bdcf8 to
7514eb3
Compare
|
Should this be backported to |
Follow-up from nodejs#18800 Code that tries to exercise tty fds must be placed in `/pseudo-tty/`. PR-URL: nodejs#18886 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This utility is fairly generic and likely useful for more than one test. PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Follow-up from nodejs#18800 Code that tries to exercise tty fds must be placed in `/pseudo-tty/`. PR-URL: nodejs#18886 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This utility is fairly generic and likely useful for more than one test. PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Follow-up from nodejs#18800 Code that tries to exercise tty fds must be placed in `/pseudo-tty/`. PR-URL: nodejs#18886 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
should these tty updates be backported to v8.x? |
|
does not land cleanly in 8.x. Would need a backport PR. |
TTY tests should almost never be placed in
/parallel/. Skipping TTYtests there due to missing tty fds just means they will never be run,
ever, on any system.
This moves the
tty-get-color-depthtest to/pseudo-tty/where the testrunner will actually make a pty fd.
Also moves
getTTYfd()intocommon/index.js.Edit: this was noticed by looking at the tty coverage.
cc @BridgeAR
CI: https://ci.nodejs.org/job/node-test-pull-request/13183/
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, tty