Skip to content

test: actually test tty getColorDepth() #18800

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.

Sign up for GitHub

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

Merged
Fishrock123 merged 2 commits into from
Feb 20, 2018
Merged

test: actually test tty getColorDepth() #18800

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92bf249
test: move getTTYfd() to common helpers
Fishrock123 Feb 16, 2018
7514eb3
test: actually test tty `getColorDepth()`
Fishrock123 Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
test: move getTTYfd() to common helpers 
This utility is fairly generic and likely useful for more than one test.

PR-URL: #18800
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
@Fishrock123
Fishrock123 committed Feb 20, 2018
commit 92bf2492cd6b78b1ab26765660d912ee34bb90c6
6 changes: 6 additions & 0 deletions test/common/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,12 @@ The realpath of the testing temporary directory.

Deletes and recreates the testing temporary directory.

### getTTYfd()

Attempts to get a valid TTY file descriptor. Returns `-1` if it fails.

The TTY file descriptor is assumed to be capable of being writable.

## WPT Module

The wpt.js module is a port of parts of
Expand Down
17 changes: 17 additions & 0 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,23 @@ exports.crashOnUnhandledRejection = function() {
(err) => process.nextTick(() => { throw err; }));
};

exports.getTTYfd = function getTTYfd() {
// Do our best to grab a tty fd.
const tty = require('tty');
// Don't attempt fd 0 as it is not writable on Windows.
// Ref: ef2861961c3d9e9ed6972e1e84d969683b25cf95
const ttyFd = [1, 2, 4, 5].find(tty.isatty);
if (ttyFd === undefined) {
try {
return fs.openSync('/dev/tty');
} catch (e) {
// There aren't any tty fd's available to use.
return -1;
}
}
return ttyFd;

@BridgeAR BridgeAR Feb 15, 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.

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

@Fishrock123 Fishrock123 Feb 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@BridgeAR BridgeAR Feb 15, 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.

There is one in sequential/test-async-wrap-getasyncid.js. And zero was removed because it can cause issues on Windows.

@Fishrock123 Fishrock123 Feb 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That part of that test definitely never ever runs. 😬

@Fishrock123 Fishrock123 Feb 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

};

// Hijack stdout and stderr
const stdWrite = {};
function hijackStdWritable(name, listener) {
Expand Down
19 changes: 1 addition & 18 deletions test/parallel/test-tty-get-color-depth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,9 @@
const common = require('../common');
const assert = require('assert').strict;
/* eslint-disable no-restricted-properties */
const { openSync } = require('fs');
const tty = require('tty');

const { WriteStream } = require('tty');

// Do our best to grab a tty fd.
function getTTYfd() {
const ttyFd = [1, 2, 4, 5].find(tty.isatty);
if (ttyFd === undefined) {
try {
return openSync('/dev/tty');
} catch (e) {
// There aren't any tty fd's available to use.
return -1;
}
}
return ttyFd;
}

const fd = getTTYfd();
const fd = common.getTTYfd();

// Give up if we did not find a tty
if (fd === -1)
Expand Down