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
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions test/parallel/test-repl-eval.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const repl = require('repl');

{
Copy link
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
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
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.

let evalCalledWithExpectedArgs = false;

const options = {
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
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
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.

})
};

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

Choose a reason for hiding this comment

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

context isn't a supply-able option?

Copy link
Member Author

Choose a reason for hiding this comment

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


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

Choose a reason for hiding this comment

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

Why the exceptions are ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

r.write('.exit\n');
}

process.on('exit', () => {
assert(evalCalledWithExpectedArgs);
});
}