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: add stdin-setrawmode.out file to accompany test #10146

Closed

Conversation

jmdarling
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Adds an accompanying .out file for test/pseudo-tty/stdin-setrawmode.js.
The test was originally merged without this file and an astute
observer found that it was causing an error message. See discussion
at #10037.

Adds an accompanying .out file for test/pseudo-tty/stdin-setrawmode.js.
The test was originally merged without this file and an astute
observer found that it was causing an error message. See discussion
at nodejs#10037.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 6, 2016
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

😄

@jmdarling
Copy link
Contributor Author

Very strange, it's passing on my machine.

Doing a full rebuild now in an effort to get it failing locally.

@Fishrock123
Copy link
Contributor

Hmm, looks like this is popping up on some platforms: process.stdin.setRawMode is not a function Odd.

@addaleax
Copy link
Member

addaleax commented Dec 6, 2016

Very strange, it's passing on my machine.

Are you running it through the test runner, or are you just executing the file? You will want to execute python tools/test.py pseudo-tty/stdin-setrawmode for properly testing pseudo-tty tests.

@Fishrock123
Copy link
Contributor

The test passes fine for me now on OS X 10.10.5

@jmdarling
Copy link
Contributor Author

I'm running it with make test. @addaleax, thanks for the tip!

Unfortunately, it's still passing for me after a fresh build using both make test and python tools/test.py pseudo-tty/stdin-setrawmode on macOS 10.12.1.

@addaleax
Copy link
Member

addaleax commented Dec 6, 2016

Yeah, sorry, my bad. Inside of CI, stdin is currently not a TTY, even for the pseudo-tty tests… sigh
echo|python tools/test.py pseudo-tty/stdin-setrawmode fails as seen in CI.

@mscdex mscdex added the tty Issues and PRs related to the tty subsystem. label Dec 6, 2016
addaleax added a commit to addaleax/node that referenced this pull request Dec 9, 2016
addaleax added a commit that referenced this pull request Dec 9, 2016
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax
Copy link
Member

addaleax commented Dec 9, 2016

Landed in b8fc9a3 as part of #10149, thanks!

@addaleax addaleax closed this Dec 9, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Dec 26, 2016
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos
Copy link
Member

targos commented Dec 26, 2016

marking dont-land-on- in favor of #10149

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
Ref: #10037
Ref: #10146
PR-URL: #10149
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants