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

gergelyke · 2017-09-07T07:23:15Z

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

lpinca · 2017-09-07T20:30:28Z

test/parallel/test-tty-backwards-api.js

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

lpinca · 2017-09-07T20:31:25Z

test/parallel/test-tty-backwards-api.js

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

gergelyke · 2017-09-08T04:40:05Z

@lpinca should be all fixed :)

BridgeAR · 2017-09-09T19:46:58Z

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

BridgeAR · 2017-09-09T21:40:50Z

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

gergelyke · 2017-09-10T06:47:45Z

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

gergelyke · 2017-09-10T07:36:59Z

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 · 2017-09-11T03:45:14Z

@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 · 2017-09-11T12:33:30Z

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

gergelyke · 2017-09-11T12:39:27Z

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 · 2017-09-11T13:18:15Z

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

gergelyke · 2017-09-11T15:17:00Z

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 · 2017-09-13T06:57:39Z

@lpinca @BridgeAR should be fine now! :)

lpinca · 2017-09-13T07:07:39Z

test/parallel/test-tty-backwards-api.js

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

BridgeAR · 2017-09-14T20:54:47Z

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

BridgeAR · 2017-09-19T20:26:19Z

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

lpinca · 2017-09-20T05:37:53Z

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

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 · 2017-09-20T13:43:48Z

Landed in 750c080

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>

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>

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>

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

lpinca approved these changes Sep 8, 2017

View reviewed changes

lpinca reviewed Sep 13, 2017

View reviewed changes

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

Copy link

Member

lpinca Sep 13, 2017

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.

test: backward compatible api for tty

3c85362

jasnell approved these changes Sep 20, 2017

View reviewed changes

watilde approved these changes Sep 20, 2017

View reviewed changes

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

Node.js 8.6.0 Release Proposal #15509

Merged

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 added the land-on-v6.x label Oct 16, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

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

v4.8.6 proposal #16500

Merged

Uh oh!

Conversation

gergelyke commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

lpinca Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

gergelyke commented Sep 8, 2017

Uh oh!

BridgeAR commented Sep 9, 2017

Uh oh!

BridgeAR commented Sep 9, 2017

Uh oh!

gergelyke commented Sep 10, 2017

Uh oh!

gergelyke commented Sep 10, 2017

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

gergelyke commented Sep 11, 2017

Uh oh!

gergelyke commented Sep 11, 2017

Uh oh!

lpinca commented Sep 11, 2017

Uh oh!

gergelyke commented Sep 11, 2017

Uh oh!

gergelyke commented Sep 13, 2017

Uh oh!

lpinca Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 14, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

lpinca commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gergelyke commented Sep 7, 2017 •

edited

Loading