Skip to content

test: add stdin-setrawmode.out file to accompany test#10146

Closed
jmdarling wants to merge 1 commit intonodejs:masterfrom
jmdarling:fix-pseudo-tty-std-setrawmode-out
Closed

test: add stdin-setrawmode.out file to accompany test#10146
jmdarling wants to merge 1 commit intonodejs:masterfrom
jmdarling:fix-pseudo-tty-std-setrawmode-out

Conversation

@jmdarling
Copy link

@jmdarling jmdarling commented Dec 6, 2016

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.

test: add stdin-setrawmode.out file
0c3914f 
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
@jmdarling jmdarling mentioned this pull request Dec 6, 2016
Closed
3 tasks
Fishrock123
Fishrock123 approved these changes Dec 6, 2016
View reviewed changes
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.

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

cjihrig
cjihrig approved these changes Dec 6, 2016
View reviewed changes
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
Author

jmdarling commented Dec 6, 2016

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

Fishrock123 commented Dec 6, 2016

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

Fishrock123 commented Dec 6, 2016

The test passes fine for me now on OS X 10.10.5

@jmdarling
Copy link
Author

jmdarling commented Dec 6, 2016

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.

@addaleax addaleax mentioned this pull request Dec 6, 2016
Closed
2 tasks
@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
test: set stdin too for pseudo-tty tests
e5ae21b 
Ref: nodejs#10037
Ref: nodejs#10146
addaleax added a commit that referenced this pull request Dec 9, 2016
@addaleax
test: set stdin too for pseudo-tty tests
def6dfb 
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
@addaleax @Fishrock123
test: set stdin too for pseudo-tty tests
f5347ab 
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
@addaleax @targos
test: set stdin too for pseudo-tty tests
e057925 
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

@targos targos mentioned this pull request Dec 26, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@addaleax @MylesBorins
test: set stdin too for pseudo-tty tests
a5fdd6a 
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
@addaleax @MylesBorins
test: set stdin too for pseudo-tty tests
e3490bd 
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
@addaleax @MylesBorins
test: set stdin too for pseudo-tty tests
42da81e 
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

Reviewers

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jmdarling @Fishrock123 @addaleax @targos @cjihrig @mscdex @nodejs-github-bot