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

test: improve readline test coverage for tty by claudiorodriguez · Pull Request #12064 · nodejs/node · GitHub

/ node Public

test: improve readline test coverage for tty - #12064

Closed
claudiorodriguez wants to merge 1 commit into
nodejs:masterfrom
claudiorodriguez:readline-more-tests
ConversationCommits (1)ChecksFiles changed
Closed

test: improve readline test coverage for tty#12064
claudiorodriguez wants to merge 1 commit into
nodejs:masterfrom
claudiorodriguez:readline-more-tests

Conversation

claudiorodriguez commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

Adds the following tests for tty readline:

  • go to beginning and end of line
  • wordLeft
  • wordRight
  • deleteWordLeft
  • deleteWordRight
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

claudiorodriguez added readline Issues and PRs related to the built-in readline module. test Issues and PRs related to the tests. labels Mar 27, 2017

Fishrock123 commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

So, to be clear... this test file runes with stdio as pipes, not ttys.

By the look of the code this doesn't actually touch TTY at all, maybe that should be removed form the commit message?

Fishrock123 commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

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

claudiorodriguez commented Mar 27, 2017

Copy link
Copy Markdown
Contributor Author

@Fishrock123 sorry, what I meant is that it improves coverage for readline when going through _ttyWrite, that is, when the Interface is constructed with terminal: true. The commit message might cause confusion with the tty module, I can definitely remove it.

Fishrock123 commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

if _ttyWrite fits into the message that might be ideal?

claudiorodriguez commented Mar 27, 2017

Copy link
Copy Markdown
Contributor Author

@Fishrock123 sure thing, I'll wait for the CI run to complete then fix that

claudiorodriguez commented Apr 18, 2017

Copy link
Copy Markdown
Contributor Author

@Fishrock123 completely forgot about this, does the new message seem alright to you?

BridgeAR approved these changes Aug 26, 2017
View reviewed changes

BridgeAR left a comment

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

LG if CI is green. This needs a rebase though

BridgeAR commented Aug 26, 2017

Copy link
Copy Markdown
Member

@claudiorodriguez would you be so kind and rebase this?

claudiorodriguez commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR rebased, cheers
New CI run: https://ci.nodejs.org/job/node-test-pull-request/9957/

BridgeAR requested changes Sep 8, 2017
View reviewed changes

BridgeAR left a comment

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

Still LGTM but it would be nice if my two comments would be addressed before landing.

Comment thread test/parallel/test-readline-interface.js Outdated

BridgeAR Sep 8, 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

Please use common.mustCall instead of called in all of these functions.

Comment thread test/parallel/test-readline-interface.js Outdated

BridgeAR Sep 8, 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

Super tiny nit - would you be so kind and upper case all beginnings of comments?

test: improve readline test coverage for tty
28adcc4 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

claudiorodriguez commented Sep 9, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR comments addressed, cheers

BridgeAR commented Sep 20, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 23, 2017

Copy link
Copy Markdown
Member

Landed in 7a95392

BridgeAR closed this Sep 23, 2017
BridgeAR pushed a commit that referenced this pull request Sep 23, 2017
test: improve readline test coverage for tty
7a95392 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

PR-URL: #12064
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
test: improve readline test coverage for tty
9c9d209 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

PR-URL: nodejs/node#12064
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
test: improve readline test coverage for tty
eccf503 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

PR-URL: #12064
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
test: improve readline test coverage for tty
b574cbf 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

PR-URL: #12064
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
test: improve readline test coverage for tty
1483ebd 
Adds the following tests for tty readline:
- go to beginning and end of line
- wordLeft
- wordRight
- deleteWordLeft
- deleteWordRight

PR-URL: #12064
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@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

jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

readline Issues and PRs related to the built-in readline module. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL