FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: backward compatible api for tty by gergelyke · Pull Request #15235 · nodejs/node · GitHub

/ node Public

test: backward compatible api for tty - #15235

Closed
gergelyke wants to merge 1 commit into
nodejs:masterfrom
gergelyke:master
ConversationCommits (1)ChecksFiles changed
Closed

test: backward compatible api for tty#15235
gergelyke wants to merge 1 commit into
nodejs:masterfrom
gergelyke:master

Conversation

gergelyke commented Sep 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor
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)

tty

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 7, 2017
mscdex added the tty Issues and PRs related to the tty subsystem. label Sep 7, 2017
lpinca reviewed Sep 7, 2017
View reviewed changes
Comment thread test/parallel/test-tty-backwards-api.js Outdated

lpinca Sep 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: this should be the first module to be required.

Comment thread test/parallel/test-tty-backwards-api.js Outdated

lpinca Sep 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: space after { and before }. ReadStream doesn't seem to be used in the test.

gergelyke commented Sep 8, 2017

Copy link
Copy Markdown
Contributor Author

@lpinca should be all fixed :)

BridgeAR commented Sep 9, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 9, 2017

Copy link
Copy Markdown
Member

@gergelyke seems like the test is constantly failing. Please take another look.

gergelyke commented Sep 10, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR can you help me what's the exact cause of the failure? cannot really find it in the logs :/

gergelyke commented Sep 10, 2017

Copy link
Copy Markdown
Contributor Author

this is the error message I get locally:

Building addon /Users/gergelyke/Development/risingstack/node/test/addons-napi/test_instanceof/
gyp: binding.gyp not found (cwd: /Users/gergelyke/Development/risingstack/node/test/addons-napi/test_instanceof) while trying to load binding.gyp
make[1]: *** [test/addons-napi/.buildstamp] Error 1
make: *** [test] Error 2

seems unrelated to my changes

BridgeAR commented Sep 11, 2017

Copy link
Copy Markdown
Member

@gergelyke this is the error

not ok 1510 parallel/test-tty-backwards-api
  ---
  duration_ms: 0.270
  severity: fail
  stack: |-
    tty.js:73
        handle: new TTY(fd, false),
                ^
    
    Error: EINVAL: invalid argument, uv_tty_init
        at new WriteStream (tty.js:73:13)
        at methods.forEach (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1604-32/test/parallel/test-tty-backwards-api.js:17:23)
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1604-32/test/parallel/test-tty-backwards-api.js:15:9)

Try to run the tests again. They should work properly.

gergelyke commented Sep 11, 2017

Copy link
Copy Markdown
Contributor Author

Ahh, had to run make test-addons-clean - on it!

gergelyke commented Sep 11, 2017

Copy link
Copy Markdown
Contributor Author

still the same issue :/

Building addon /Users/gergelyke/Development/risingstack/node/test/addons-napi/test_instanceof/
gyp: binding.gyp not found (cwd: /Users/gergelyke/Development/risingstack/node/test/addons-napi/test_instanceof) while trying to load binding.gyp
make[1]: *** [test/addons-napi/.buildstamp] Error 1
make: *** [test] Error 2

@trevnorris any chance you have a clue what could go wrong?

lpinca commented Sep 11, 2017

Copy link
Copy Markdown
Member

@gergelyke try to manually delete the test folders. See #13582.

gergelyke commented Sep 11, 2017

Copy link
Copy Markdown
Contributor Author

That helped, thanks. However, I don't really understand the problem here.

Based on the libuv docs (http://docs.libuv.org/en/v1.x/tty.html#c.uv_tty_init), the integer should be either 0, 1 or 2. Any idea why it fails?

gergelyke commented Sep 13, 2017

Copy link
Copy Markdown
Contributor Author

@lpinca @BridgeAR should be fine now! :)

lpinca reviewed Sep 13, 2017
View reviewed changes
Comment thread test/parallel/test-tty-backwards-api.js Outdated

lpinca Sep 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd remove this as it seems to be no longer used.

BridgeAR commented Sep 14, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member

@lpinca would you be so kind and re-approve?

lpinca commented Sep 20, 2017

Copy link
Copy Markdown
Member

Still LGTM but it would be nice if there were more approval.
Ping @nodejs/collaborators.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Sep 20, 2017
test: backward compatible api for tty
750c080 
PR-URL: nodejs#15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

BridgeAR commented Sep 20, 2017

Copy link
Copy Markdown
Member

Landed in 750c080

BridgeAR closed this Sep 20, 2017
jasnell pushed a commit that referenced this pull request Sep 20, 2017
test: backward compatible api for tty
d82ae0c 
PR-URL: #15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
test: backward compatible api for tty
a9c676c 
PR-URL: nodejs/node#15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
test: backward compatible api for tty
eeebf22 
PR-URL: nodejs/node#15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
test: backward compatible api for tty
72bf0b4 
PR-URL: #15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
test: backward compatible api for tty
5c99fc3 
PR-URL: #15235
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins mentioned this pull request Nov 3, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
jasnell jasnell approved these changes
watilde watilde approved these changes

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.

8 participants

Back | FazBrowse Home | New Git URL