test: skip whether TTY is availble#13991

trevnorris · 2017-06-29T21:23:21Z

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

Description

If TTY isn't available then the test will always fail.

Fixes: #13984

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

addaleax

LGTM, but at some point I’d like it if we could split up the async_hooks test directory and move these to the pseudo-tty tests

trevnorris · 2017-07-03T21:15:12Z

@refack This conflicted with test-ttywrap.readstream.js from 372b85d. I made the change of how the tty fd is found to match test-ttywrap.writestream.js.

CI: https://ci.nodejs.org/job/node-test-commit/10936/

refack · 2017-07-03T21:21:24Z

@refack This conflicted with test-ttywrap.readstream.js from 372b85d. I made the change of how the tty fd is found to match test-ttywrap.writestream.js.

New code works here (on Windows), but I see you have problems in linux

not ok 42 async-hooks/test-ttywrap.readstream --- duration_ms: 0.57 severity: fail stack: |- assert.js:55 throw new errors.AssertionError({^ AssertionError [ERR_ASSERTION]: null !== null at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/test-ttywrap.readstream.js:19:8) at Module._compile (module.js:569:30) at Object.Module._extensions..js (module.js:580:10) at Module.load (module.js:503:32) at tryModuleLoad (module.js:466:12) at Function.Module._load (module.js:458:3) at Function.Module.runMain (module.js:605:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:575:3 ... ok 43 async-hooks/test-querywrap --- duration_ms: 0.265 ... not ok 44 async-hooks/test-ttywrap.writestream --- duration_ms: 0.75 severity: fail stack: |- assert.js:55 throw new errors.AssertionError({^ AssertionError [ERR_ASSERTION]: null !== null at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/test-ttywrap.writestream.js:19:8) at Module._compile (module.js:569:30) at Object.Module._extensions..js (module.js:580:10) at Module.load (module.js:503:32) at tryModuleLoad (module.js:466:12) at Function.Module._load (module.js:458:3) at Function.Module.runMain (module.js:605:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:575:3 ...

https://ci.nodejs.org/job/node-test-commit-linuxone/7060/nodes=rhel72-s390x/console

refack · 2017-07-03T21:43:40Z

test/async-hooks/test-ttywrap.readstream.js

This i'm not in love with... does require('tty') have significant side effects?

Hm. Reason I return null here then check it below is because I'd have imagined that common.getTTYfd() should only return a positive number if there's a valid fd for the TTY. Thus ReadStream/WriteStream shouldn't fail. Appears that isn't always the case. So I'll go back to the null check and early return.
I will defend the use of getTTYfd(), which also does a require('tty') when called. So I'm not worried about the possible side effects of calling it here.

So if we don't fear side effect, why not move the require to the top?
The invoked lambda I understand (return null and assign to const is very functional-programing, and I like 👍 )

refack · 2017-07-03T21:52:25Z

test/async-hooks/test-ttywrap.writestream.js

Ohh here it is again.

refack · 2017-07-03T21:54:43Z

Is it easy to parameterize and merge these two?

trevnorris · 2017-07-05T22:36:36Z

@refack

Is it easy to parameterize and merge these two?

Probably. If not in this PR I'll add another that merges them later.

trevnorris · 2017-07-06T09:32:24Z

@refack Don't know why I didn't think of the fact that process.{stdin,stdout} are already setup for this. Made changes and running CI again.

CI: https://ci.nodejs.org/job/node-test-commit/10980/

If TTY isn't available then the test will always fail. Also use the already available process.stdin instead of opening another ReadStream. Fixes: nodejs#13984 PR-URL: nodejs#13991

Match changes made to test-ttywrap.readstream for consistency. PR-URL: nodejs#13991

refack

K.I.S.S. FTW!

AndreasMadsen · 2017-07-10T20:38:43Z

Anything preventing this from landing?

refack · 2017-07-10T20:59:35Z

Anything preventing this from landing?

I believe just etiquette, to customarily let Collaborators land their own PRs 🤷‍♂️
CI was green, so LGTM.

AndreasMadsen · 2017-07-10T21:04:39Z

I believe just etiquette, to customarily let Collaborators land their own PRs.

That is why I asked :)

jasnell · 2017-07-12T15:25:16Z

I believe just etiquette, to customarily let Collaborators land their own PRs.

Yes and no. Anyone should feel free to land any PR that is ready to land, particularly ones that are more or less trivial. For PRs that are more involved, then yes, it's best to let the authoring contributor land as they have the best context to know when it's ready to go.

If TTY isn't available then the test will always fail. Also use the already available process.stdin instead of opening another ReadStream. PR-URL: nodejs#13991Fixes: nodejs#13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Match changes made to test-ttywrap.readstream for consistency. PR-URL: nodejs#13991Fixes: nodejs#13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

refack · 2017-07-12T18:45:58Z

Since @trevnorris is busy with another PR, and I'm pretty aware of the context:
Landed in 3b4010b250d50b

trevnorris · 2017-07-12T20:37:07Z

@refack Thanks for landing this.

If TTY isn't available then the test will always fail. Also use the already available process.stdin instead of opening another ReadStream. PR-URL: #13991Fixes: #13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Match changes made to test-ttywrap.readstream for consistency. PR-URL: #13991Fixes: #13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

If TTY isn't available then the test will always fail. Also use the already available process.stdin instead of opening another ReadStream. PR-URL: #13991Fixes: #13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Match changes made to test-ttywrap.readstream for consistency. PR-URL: #13991Fixes: #13984 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

trevnorris requested a review from addaleax June 29, 2017 21:23

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jun 29, 2017

trevnorris mentioned this pull request Jun 29, 2017
test-ttywrap.readstream does not check if a valid TTY fd is available #13984
Closed

trevnorris force-pushed the fix-13984 branch from b40288f to 514108aCompare June 29, 2017 21:27

addaleax approved these changes Jun 29, 2017
View reviewed changes

trevnorris force-pushed the fix-13984 branch from 514108a to 8e8b8aaCompare June 29, 2017 21:31

jasnell approved these changes Jun 30, 2017
View reviewed changes

trevnorris force-pushed the fix-13984 branch from 8e8b8aa to 66e84b1Compare July 3, 2017 21:12

trevnorris requested a review from refack July 3, 2017 21:12

refack reviewed Jul 3, 2017
View reviewed changes

trevnorris force-pushed the fix-13984 branch from 66e84b1 to e790464Compare July 6, 2017 09:31

trevnorris force-pushed the fix-13984 branch from e790464 to 6793244Compare July 6, 2017 09:50

trevnorris added 2 commits July 6, 2017 03:50

test,async_hooks: skip whether TTY is availble
858808e
If TTY isn't available then the test will always fail. Also use the already available process.stdin instead of opening another ReadStream. Fixes: nodejs#13984 PR-URL: nodejs#13991

test,async_hooks: match test-ttywrap.readstream
2a0647e
Match changes made to test-ttywrap.readstream for consistency. PR-URL: nodejs#13991

trevnorris force-pushed the fix-13984 branch from 6793244 to 2a0647eCompare July 6, 2017 09:52

refack approved these changes Jul 6, 2017
View reviewed changes

refack closed this Jul 12, 2017

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

addaleax mentioned this pull request Jul 18, 2017
v8.2.0 proposal #13744
Merged

Uh oh!

test: skip whether TTY is availble#13991

test: skip whether TTY is availble #13991

Uh oh!

Conversation

trevnorris commented Jun 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Jul 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 3, 2017

Uh oh!

refackJul 3, 2017

Choose a reason for hiding this comment

Uh oh!

trevnorrisJul 5, 2017

Choose a reason for hiding this comment

Uh oh!

refackJul 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackJul 3, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 3, 2017

Uh oh!

trevnorris commented Jul 5, 2017

Uh oh!

trevnorris commented Jul 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 10, 2017

Uh oh!

refack commented Jul 10, 2017

Uh oh!

AndreasMadsen commented Jul 10, 2017

Uh oh!

jasnell commented Jul 12, 2017

Uh oh!

refack commented Jul 12, 2017

Uh oh!

trevnorris commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

trevnorris commented Jun 29, 2017•
edited
Loading

trevnorris commented Jul 3, 2017•
edited
Loading

refackJul 5, 2017•
edited
Loading

trevnorris commented Jul 6, 2017•
edited
Loading