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

+  return common.skip('no valid TTY fd available');
+const ttyStream = (() => {
+  try {
+    return new (require('tty').ReadStream)(tty_fd);


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

@@ -16,47 +16,39 @@ const ttyStream = (() => {
    return null;


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#13991 Fixes: 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#13991 Fixes: 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 3b4010b 250d50b

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: #13991 Fixes: #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: #13991 Fixes: #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: #13991 Fixes: #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: #13991 Fixes: #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 514108a Compare 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 8e8b8aa Compare 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 66e84b1 Compare 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 e790464 Compare July 6, 2017 09:31

trevnorris force-pushed the fix-13984 branch from e790464 to 6793244 Compare 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 2a0647e Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skip whether TTY is availble #13991

test: skip whether TTY is availble #13991

trevnorris commented Jun 29, 2017 •

edited

Loading

addaleax left a comment

trevnorris commented Jul 3, 2017 •

edited

Loading

refack commented Jul 3, 2017

refack Jul 3, 2017

trevnorris Jul 5, 2017

refack Jul 5, 2017 •

edited

Loading

refack Jul 3, 2017

refack commented Jul 3, 2017

trevnorris commented Jul 5, 2017

trevnorris commented Jul 6, 2017 •

edited

Loading

refack left a comment

AndreasMadsen commented Jul 10, 2017

refack commented Jul 10, 2017

AndreasMadsen commented Jul 10, 2017

jasnell commented Jul 12, 2017

refack commented Jul 12, 2017

trevnorris commented Jul 12, 2017

test: skip whether TTY is availble #13991

test: skip whether TTY is availble #13991

Conversation

trevnorris commented Jun 29, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Description

addaleax left a comment

Choose a reason for hiding this comment

trevnorris commented Jul 3, 2017 • edited Loading

refack commented Jul 3, 2017

refack Jul 3, 2017

Choose a reason for hiding this comment

trevnorris Jul 5, 2017

Choose a reason for hiding this comment

refack Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

refack Jul 3, 2017

Choose a reason for hiding this comment

refack commented Jul 3, 2017

trevnorris commented Jul 5, 2017

trevnorris commented Jul 6, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 10, 2017

refack commented Jul 10, 2017

AndreasMadsen commented Jul 10, 2017

jasnell commented Jul 12, 2017

refack commented Jul 12, 2017

trevnorris commented Jul 12, 2017

trevnorris commented Jun 29, 2017 •

edited

Loading

trevnorris commented Jul 3, 2017 •

edited

Loading

refack Jul 5, 2017 •

edited

Loading

trevnorris commented Jul 6, 2017 •

edited

Loading