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: improve readline test coverage for tty #12064

Closed
wants to merge 1 commit into from
Closed

test: improve readline test coverage for tty #12064

wants to merge 1 commit into from

Conversation

claudiorodriguez
Copy link
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 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
Copy link
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
Copy link
Contributor

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

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

if _ttyWrite fits into the message that might be ideal?

@claudiorodriguez
Copy link
Contributor Author

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

@claudiorodriguez
Copy link
Contributor Author

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

BridgeAR
BridgeAR approved these changes Aug 26, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LG if CI is green. This needs a rebase though

@BridgeAR
Copy link
Member

@claudiorodriguez would you be so kind and rebase this?

@claudiorodriguez
Copy link
Contributor Author

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

BridgeAR
BridgeAR requested changes Sep 8, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

test/parallel/test-readline-interface.js Outdated
rli.on('line', function(line) {
called = true;
assert.strictEqual(line, 'the quick brown fox');
});
Copy link
Member

Choose a reason for hiding this comment

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

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

test/parallel/test-readline-interface.js Outdated
assert.ok(called);
rli.close();

// no effect if pressed at end of line
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@BridgeAR comments addressed, cheers

@BridgeAR
Copy link
Member

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

@BridgeAR
Copy link
Member

Landed in 7a95392

@BridgeAR BridgeAR closed this Sep 23, 2017
BridgeAR pushed a commit that referenced this pull request Sep 23, 2017
@claudiorodriguez @BridgeAR
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
@claudiorodriguez @addaleax
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
@claudiorodriguez @jasnell
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
@claudiorodriguez @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@claudiorodriguez @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell 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
@claudiorodriguez @Fishrock123 @BridgeAR @jasnell @MylesBorins