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: add test for repl.defineCommand #3908

Closed
wants to merge 1 commit into from
Closed

test: add test for repl.defineCommand #3908

wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Nov 19, 2015

There didn't appear to be a test for REPLServer#defineCommand, probably because it wasn't documented before, so here's a test for it.

@thefourtheye thefourtheye added repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests. labels Nov 19, 2015
@thefourtheye
Copy link
Contributor

This is awesome. CI: https://ci.nodejs.org/job/node-test-pull-request/776/

jasnell
jasnell reviewed Nov 19, 2015
View reviewed changes
test/parallel/test-repl-definecommand.js
var stream = require('stream'),
assert = require('assert'),
repl = require('repl');

Copy link
Member

Choose a reason for hiding this comment

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

For consistency, please use const here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure no prob. I saw there was a lint error in CI so I'll fix that and this.

Should I const everything in this file that can be? Or just the requires?

Copy link
Member

Choose a reason for hiding this comment

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

at least the requires for sure. make sure it passes lint. the rest I'd consider optional

Copy link
Contributor

Choose a reason for hiding this comment

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

@bengl You can actually check everything locally simply by running make -j4 test, here 4 denotes the number of tasks to be done parallelly.

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

One minor nit but LGTM if CI is green

@bengl bengl force-pushed the definecommandtest branch from 77a3e52 to 2ceb942 Compare November 19, 2015 18:01
@bengl
Copy link
Member Author

bengl commented Nov 19, 2015

Cool. make -j4 test and make jslint should both passing now. Also using const in places where I can.

thefourtheye
thefourtheye reviewed Nov 19, 2015
View reviewed changes
test/parallel/test-repl-definecommand.js
});

inputStream.write('.help\n');
assert(/\nsay1\thelp for say1\n/.test(output), 'help for say1 present');
Copy link
Contributor

Choose a reason for hiding this comment

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

The second parameter will be thrown as an error, if the first param is not truthy. So the text should be negative I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sure. Will change those messages to be negative.

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

Awesome... doing another CI run just to be safe: https://ci.nodejs.org/job/node-test-pull-request/788/

@bengl
test: add test for repl.defineCommand
db28b50 
It also tests displayPrompt by checking for '> '.
@bengl bengl force-pushed the definecommandtest branch from 2ceb942 to db28b50 Compare November 19, 2015 18:25
@bengl
Copy link
Member Author

bengl commented Nov 19, 2015

Changed error messages to be negative.

@thefourtheye
Copy link
Contributor

LGTM

@bengl
Copy link
Member Author

bengl commented Nov 19, 2015

@jasnell looks like CI failed because I force-pushed a new version of it too early. Perhaps kick it off again?

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

heh. Ok... new run: https://ci.nodejs.org/job/node-test-pull-request/790/

@bengl
Copy link
Member Author

bengl commented Nov 19, 2015

Looks like some unrelated failures in Windows and FreeBSD.

@Fishrock123
Copy link
Contributor

Thanks! Landed in e5d97fd

@Fishrock123 Fishrock123 closed this Dec 1, 2015
Fishrock123 pushed a commit that referenced this pull request Dec 1, 2015
@bengl @Fishrock123
test: add test for repl.defineCommand()
e5d97fd 
It also tests displayPrompt by checking for '> '.

PR-URL: #3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
@bengl @rvagg
test: add test for repl.defineCommand()
4302648 
It also tests displayPrompt by checking for '> '.

PR-URL: #3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
This was referenced Dec 8, 2015
Closed
Merged
MylesBorins pushed a commit that referenced this pull request Dec 15, 2015
@bengl
test: add test for repl.defineCommand()
04e1e9c 
It also tests displayPrompt by checking for '> '.

PR-URL: #3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
Closed
jasnell pushed a commit that referenced this pull request Dec 17, 2015
@bengl @jasnell
test: add test for repl.defineCommand()
4e46e04 
It also tests displayPrompt by checking for '> '.

PR-URL: #3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
@bengl @jasnell
test: add test for repl.defineCommand()
51b5f42 
It also tests displayPrompt by checking for '> '.

PR-URL: #3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@bengl
test: add test for repl.defineCommand()
41cde38 
It also tests displayPrompt by checking for '> '.

PR-URL: nodejs#3908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem. 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
@bengl @thefourtheye @jasnell @Fishrock123 @MylesBorins