test: actually test tty getColorDepth() #18800

Fishrock123 · 2018-02-15T16:41:54Z

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.

Also moves getTTYfd() into common/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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test, tty

BridgeAR

LGTM with two suggestions. Thanks for fixing this.

BridgeAR · 2018-02-15T16:48:32Z

test/pseudo-tty/test-tty-get-color-depth.js

+'use strict';
+
+const common = require('../common');
+const assert = require('assert');


I would like that we keep using strict mode. It is something that I would like to switch over on the long term.

@BridgeAR That lint rule does not seem to be correctly applied to /pseudo-tty/, so the linter complains.

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.

BridgeAR · 2018-02-15T16:49:34Z

test/common/index.js

+      return -1;
+    }
+  }
+  return ttyFd;


Something similar wa sin here at some point and is used in another test file. It would be good to consolidate in that case.

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 is one in sequential/test-async-wrap-getasyncid.js. And zero was removed because it can cause issues on Windows.

That part of that test definitely never ever runs. 😬

I'll move that out next and base it off of this then, I suppose.

Trott · 2018-02-16T00:00:49Z

test/common/index.js

@@ -24,6 +24,8 @@
 const process = global.process;  // Some tests tamper with the process global.
 const path = require('path');
 const fs = require('fs');
+/* eslint-disable no-restricted-properties */
+const { openSync } = require('fs');


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)?

Trott · 2018-02-16T00:04:04Z

test/common/index.js

@@ -24,6 +24,8 @@
 const process = global.process;  // Some tests tamper with the process global.
 const path = require('path');
 const fs = require('fs');
+/* eslint-disable no-restricted-properties */


This eslint-disable comment is unnecessary. Please remove it.

addaleax

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.

addaleax · 2018-02-16T13:20:34Z

test/common/index.js

+exports.getTTYfd = function getTTYfd() {
+  const tty = require('tty');
+  // Do our best to grab a tty fd.
+  const ttyFd = [1, 2, 4, 5].find(tty.isatty);


0 is also a very good candidate for this

That was removed recently: ef28619

Interesting. I'll have to leave a note about this.

Fishrock123 · 2018-02-16T16:30:24Z

/dev/tty should in practice be a pretty reliable way to get a TTY, at least for developer machines.

That's only true if a terminal is directly attached to the process.

If I comment out these lines on master, the test runner fails:

node/test/parallel/test-tty-get-color-depth.js

Lines 28 to 29 in 3e1e450

    
           if (fd === -1) 
        
             common.skip();

Fishrock123 · 2018-02-16T17:13:58Z

Updated. ~~new CI: https://ci.nodejs.org/job/node-test-pull-request/13213/~~

Actual new CI: https://ci.nodejs.org/job/node-test-pull-request/13214/

BridgeAR

Still LGTM

BridgeAR · 2018-02-18T11:17:52Z

@Fishrock123 this needs a rebase

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>

MylesBorins · 2018-02-21T15:14:49Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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>

Follow-up from #18800 Code that tries to exercise tty fds must be placed in `/pseudo-tty/`. PR-URL: #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>

MylesBorins · 2018-08-07T19:29:03Z

should these tty updates be backported to v8.x?

jasnell · 2018-08-17T20:43:20Z

does not land cleanly in 8.x. Would need a backport PR.

Fishrock123 added test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem. labels Feb 15, 2018

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

BridgeAR approved these changes Feb 15, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2018

jasnell approved these changes Feb 15, 2018

View reviewed changes

Trott reviewed Feb 16, 2018

View reviewed changes

addaleax approved these changes Feb 16, 2018

View reviewed changes

Fishrock123 force-pushed the actually-test-tty-get-color-depth branch 2 times, most recently from 667ced5 to f4b997d Compare February 16, 2018 17:12

Fishrock123 force-pushed the actually-test-tty-get-color-depth branch from f4b997d to e745f29 Compare February 16, 2018 17:17

BridgeAR approved these changes Feb 18, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2018

Fishrock123 force-pushed the actually-test-tty-get-color-depth branch from e745f29 to c8bdcf8 Compare February 20, 2018 17:26

Fishrock123 added 2 commits February 20, 2018 12:31

test: move getTTYfd() to common helpers

92bf249

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>

Fishrock123 force-pushed the actually-test-tty-get-color-depth branch from c8bdcf8 to 7514eb3 Compare February 20, 2018 17:32

Fishrock123 merged commit 7514eb3 into nodejs:master Feb 20, 2018

Fishrock123 deleted the actually-test-tty-get-color-depth branch February 20, 2018 17:34

Fishrock123 mentioned this pull request Feb 20, 2018

test: actually test the ttywrap portion of getasyncid #18886

Merged

3 tasks

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

Fishrock123 added a commit that referenced this pull request Feb 22, 2018

test: really test the ttywrap bits of getasyncid

0a26280

Follow-up from #18800 Code that tries to exercise tty fds must be placed in `/pseudo-tty/`. PR-URL: #18886 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos mentioned this pull request Apr 2, 2018

test: improve tty.getColorDepth coverage #19446

Closed

4 tasks

MylesBorins added the lts-watch-v8.x label Aug 7, 2018

jasnell added the backport-requested-v8.x label Aug 17, 2018

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: actually test tty getColorDepth() #18800

test: actually test tty getColorDepth() #18800

Fishrock123 commented Feb 15, 2018 •

edited

Loading

BridgeAR left a comment

BridgeAR Feb 15, 2018

Fishrock123 Feb 15, 2018 •

edited

Loading

BridgeAR Feb 15, 2018

BridgeAR Feb 15, 2018

Fishrock123 Feb 15, 2018

BridgeAR Feb 15, 2018

Fishrock123 Feb 15, 2018

Fishrock123 Feb 15, 2018

Trott Feb 16, 2018 •

edited

Loading

Trott Feb 16, 2018

addaleax left a comment

addaleax Feb 16, 2018

BridgeAR Feb 16, 2018

Fishrock123 Feb 16, 2018

Fishrock123 commented Feb 16, 2018

Fishrock123 commented Feb 16, 2018 •

edited

Loading

BridgeAR left a comment

BridgeAR commented Feb 18, 2018

MylesBorins commented Feb 21, 2018

MylesBorins commented Aug 7, 2018

jasnell commented Aug 17, 2018

test: actually test tty getColorDepth() #18800

test: actually test tty getColorDepth() #18800

Conversation

Fishrock123 commented Feb 15, 2018 • edited Loading

Checklist

Affected core subsystem(s)

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Feb 16, 2018

Fishrock123 commented Feb 16, 2018 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 18, 2018

MylesBorins commented Feb 21, 2018

MylesBorins commented Aug 7, 2018

jasnell commented Aug 17, 2018

Fishrock123 commented Feb 15, 2018 •

edited

Loading

Fishrock123 Feb 15, 2018 •

edited

Loading

Trott Feb 16, 2018 •

edited

Loading

Fishrock123 commented Feb 16, 2018 •

edited

Loading