Skip to content
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.

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

test-ttywrap.readstream does not check if a valid TTY fd is available #13984

Closed
psmarshall opened this issue Jun 29, 2017 · 2 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.

Comments

@psmarshall
Copy link
Contributor

  • Version: master
  • Platform: all
  • Subsystem: tests

We have a test failure that occurs on our node build bot that we can't reproduce locally, that happens in test-ttywrap.readstream.js. We get the following results:

not ok 45 async-hooks/test-ttywrap.readstream
  duration_ms: 0.89
  severity: fail
  stack: |-
    tty.js:47
        handle: new TTY(fd, true)
                ^
    
    Error: EINVAL: invalid argument, uv_tty_init
        at new ReadStream (tty.js:47:13)
        at Object.<anonymous> (*..snip..*/node.js/test/async-hooks/test-ttywrap.readstream.js:13:19)
        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 46 async-hooks/test-ttywrap.writestream # skip no valid TTY fd available

test-ttywrap.writestream.js seems to check whether the TTY fd is available and then skip if not - does it make sense to do the same thing in the readstream test?

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem. labels Jun 29, 2017
@addaleax
Copy link
Member

test-ttywrap.writestream.js seems to check whether the TTY fd is available and then skip if not - does it make sense to do the same thing in the readstream test?

I would think so, yes.

@trevnorris
Copy link
Contributor

Have fix at: #13991

trevnorris added a commit to trevnorris/node that referenced this issue Jul 6, 2017
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
refack pushed a commit to refack/node that referenced this issue Jul 12, 2017
Match changes made to test-ttywrap.readstream for consistency.

PR-URL: nodejs#13991
Fixes: nodejs#13984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack closed this as completed in 250d50b Jul 12, 2017
addaleax pushed a commit that referenced this issue Jul 18, 2017
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Match changes made to test-ttywrap.readstream for consistency.

PR-URL: #13991
Fixes: #13984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Match changes made to test-ttywrap.readstream for consistency.

PR-URL: #13991
Fixes: #13984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants