Skip to content

test: add test for repl.defineCommand#3908

Closed
bengl wants to merge 1 commit intonodejs:masterfrom
bengl:definecommandtest
Closed

test: add test for repl.defineCommand#3908
bengl wants to merge 1 commit intonodejs:masterfrom
bengl:definecommandtest

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

thefourtheye commented Nov 19, 2015

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 Outdated
Copy link
Member

@jasnell jasnell Nov 19, 2015

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

@bengl bengl Nov 19, 2015

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

@jasnell jasnell Nov 19, 2015

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

@thefourtheye thefourtheye Nov 19, 2015

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

@thefourtheye thefourtheye Nov 19, 2015

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

@bengl bengl Nov 19, 2015

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

thefourtheye commented Nov 19, 2015

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

Fishrock123 commented Dec 1, 2015

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