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.

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

test: minimal repl eval option test #5192

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@Trott
Copy link
Member

commented Feb 11, 2016

Fixes: #3544

@Trott

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

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

{

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Feb 11, 2016

Contributor

Why this block is necessary?

This comment has been minimized.

Copy link
@Trott

Trott Feb 11, 2016

Author Member

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).

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 11, 2016

Member

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


try {
r.write('foo\n');
} finally {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Feb 11, 2016

Contributor

Why the exceptions are ignored?

This comment has been minimized.

Copy link
@Trott

Trott Feb 11, 2016

Author Member

Unless I'm mistaken, they'll only be ignored if there's a catch block. Since there's no catch block, the exceptions are propagated normally. (Tested by putting a throw call inside the try block.)

This comment has been minimized.

Copy link
@Trott

Trott Feb 11, 2016

Author Member

Although actually I see now that the AssertionErrors do not cause the test to exit with an error code, so that's a problem (and apologies if that's exactly what you were referring to!).

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Feb 11, 2016

Contributor

TIL :-) I always thought ignoring catch is the same as ignoring errors and I have no idea why I used to think that :( Thanks for enlightening :)

This comment has been minimized.

Copy link
@Trott

Trott Feb 11, 2016

Author Member

OK, the "does not exit with an error when it should" thing is now fixed.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

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');

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 11, 2016

Member

I think the parens here are unnecessary?

This comment has been minimized.

Copy link
@Trott

Trott Feb 11, 2016

Author Member

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

};

const r = repl.start(options);
r.context = {foo: 'bar'};

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 11, 2016

Member

context isn't a supply-able option?

This comment has been minimized.

Copy link
@Trott
@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

LGTM

@Trott

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2016

Landed in 90451a6

@Trott Trott closed this Feb 12, 2016

Trott added a commit that referenced this pull request Feb 12, 2016

test: minimal repl eval option test
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 15, 2016

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

rvagg added a commit that referenced this pull request Feb 15, 2016

test: minimal repl eval option test
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Feb 18, 2016

test: minimal repl eval option test
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Feb 18, 2016

test: minimal repl eval option test
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Feb 18, 2016

Merged

V4.4.0 proposal #5301

stefanmb added a commit to stefanmb/node that referenced this pull request Feb 23, 2016

test: minimal repl eval option test
Fixes: nodejs#3544
PR-URL: nodejs#5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 2, 2016

test: minimal repl eval option test
Fixes: #3544
PR-URL: #5192
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.