Skip to content

test: minimal repl eval option test - #5192

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:repl
Closed

Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:repl

Conversation

@Trott

@Trott Trott commented Feb 11, 2016

Copy link
Copy Markdown
Member

Fixes: #3544

@Trott Trott added repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to Node.js core tests and test infrastructure. lts-watch-v4.x labels Feb 11, 2016
@Trott

Trott commented Feb 11, 2016

Copy link
Copy Markdown
Member Author

const assert = require('assert');
const repl = require('repl');

{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this block is necessary?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's there so that if we write a subsequent test (such as to test that context is sent on tab completion), we can make sure there are no side effects (because the variables are scoped to the block).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Due to block-scoped vars (i.e. let and const) this feature, though existing in es5, is now actually useful.

@Trott

Trott commented Feb 11, 2016

Copy link
Copy Markdown
Member Author

@Fishrock123

Copy link
Copy Markdown
Contributor

LGTM

eval: common.mustCall((cmd, context) => {
// Assertions here will not cause the test to exit with an error code
// so set a boolean that is checked in process.on('exit',...) instead.
evalCalledWithExpectedArgs = (cmd === 'foo\n' && context.foo === 'bar');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the parens here are unnecessary?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's right. They're just there for clarity. If they're objectionable, I can remove them.

@jasnell

jasnell commented Feb 11, 2016

Copy link
Copy Markdown
Member

LGTM

@Trott

Trott commented Feb 12, 2016

Copy link
Copy Markdown
Member Author

Landed in 90451a6

@Trott Trott closed this Feb 12, 2016
Trott added a commit that referenced this pull request Feb 12, 2016
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis

Copy link
Copy Markdown
Member

Belated LGTM FWIW. Thanks for picking this up. :-)

rvagg pushed a commit that referenced this pull request Feb 15, 2016
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
Fixes: nodejs#3544
PR-URL: nodejs#5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Trott deleted the repl branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants