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.

Sign up for GitHub

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

Jump to bottom

test: backward compatible api for tty #15235

Closed
wants to merge 1 commit into from
Closed

test: backward compatible api for tty #15235

wants to merge 1 commit into from

Conversation

gergelyke
Copy link
Contributor

@gergelyke gergelyke commented Sep 7, 2017 •
edited
Loading

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 7, 2017
@mscdex mscdex added the tty Issues and PRs related to the tty subsystem. label Sep 7, 2017
lpinca
lpinca reviewed Sep 7, 2017
View reviewed changes
test/parallel/test-tty-backwards-api.js Outdated
@@ -0,0 +1,26 @@
'use strict';
const assert = require('assert');
const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

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

test/parallel/test-tty-backwards-api.js Outdated
const assert = require('assert');
const common = require('../common');

const {WriteStream, ReadStream} = require('tty');
Copy link
Member

Choose a reason for hiding this comment

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

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

@gergelyke
Copy link
Contributor Author

@lpinca should be all fixed :)

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

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

@gergelyke
Copy link
Contributor Author

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

@gergelyke
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@gergelyke
Copy link
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
Copy link
Member

lpinca commented Sep 11, 2017

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

@gergelyke
Copy link
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
Copy link
Contributor Author

@lpinca @BridgeAR should be fine now! :)

lpinca
lpinca reviewed Sep 13, 2017
View reviewed changes
test/parallel/test-tty-backwards-api.js Outdated
@@ -0,0 +1,26 @@
'use strict';
const common = require('../common');
const assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

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

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

@BridgeAR
Copy link
Member

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

@BridgeAR
Copy link
Member

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

@lpinca
Copy link
Member

lpinca commented Sep 20, 2017

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
@gergelyke @BridgeAR
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
Copy link
Member

Landed in 750c080

@BridgeAR BridgeAR closed this Sep 20, 2017
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@gergelyke @jasnell
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 jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@gergelyke
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
@gergelyke
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
@gergelyke @MylesBorins
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 MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@gergelyke @MylesBorins
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 MylesBorins mentioned this pull request Nov 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@watilde watilde watilde approved these changes

@jasnell jasnell jasnell 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
@gergelyke @BridgeAR @lpinca @jasnell @watilde @mscdex @MylesBorins @nodejs-github-bot