Skip to content

test: add tests for REPL custom evals #57691

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

Merged
aduh95 merged 4 commits into from
Apr 7, 2025
Merged

test: add tests for REPL custom evals #57691

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8b6c18
test: add tests for REPL custom evals
dario-piotrowicz Mar 30, 2025
afd03ec
Apply suggestions from code review
dario-piotrowicz Mar 31, 2025
4da7e25
move `getReplOutput` up
dario-piotrowicz Mar 31, 2025
63034cb
use `concurrency: true` and adjust tests
dario-piotrowicz Apr 5, 2025
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
Prev Previous commit
Next Next commit
move getReplOutput up
  • Loading branch information
@dario-piotrowicz
dario-piotrowicz committed Mar 31, 2025
commit 4da7e259592cd2c44b318ba465a82252b0c8cb87
44 changes: 22 additions & 22 deletions test/parallel/test-repl-custom-eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,28 @@ const { describe, it } = require('node:test');

const repl = require('repl');

function getReplOutput(input, replOptions, run = true) {
const inputStream = new ArrayStream();
const outputStream = new ArrayStream();

repl.start({
input: inputStream,
output: outputStream,
...replOptions,
});

let output = '';
outputStream.write = (chunk) => (output += chunk);

inputStream.emit('data', input);

if (run) {
inputStream.run(['']);
}

return output;
}

describe('repl with custom eval', () => {

@aduh95 aduh95 Mar 31, 2025

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.

We can probably run them in parallel when running the file standalone

Suggested change
describe('repl with custom eval', () => {
describe('repl with custom eval', { concurrency: !process.env.TEST_PARALLEL }, () => {

@dario-piotrowicz dario-piotrowicz Mar 31, 2025

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.

Sounds good 🙂

But is { concurrency: !process.env.TEST_PARALLEL } correct?

a falsy concurrency value means that the tests won't be run in parallel right? so we are saying that if TEST_PARALLEL is truthy then the tests here need to be run sequentially? or am I misunderstanding?

@aduh95 aduh95 Apr 4, 2025

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.

TEST_PARALLEL is set by Python runner when it's running test in parallel. When it is, we don't want the Node.js one to parallelize on its own, otherwise it could oversubscribe the machine – although I'm not sure if that's the case, I originally thought getReplOutput was spawning a subprocess, but if that's not the case, it shouldn't really matter

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

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.

Thanks for the explanation @aduh95 🙏 (it is still a bit murky to me but I do get the gist of it 😅)

Regarding getReplOutput no, I am quite sure that it doesn't spawn a subprocess, since it simply starts a REPLServer which does run in the same process (and it uses runInContext and runInThisContext to evalutate code)

So given the above, are you happy with the current version of the code? 🙂

@aduh95 aduh95 Apr 5, 2025 •
edited
Loading

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.

Currently it runs the test serially, so no I'm not happy with it unless we have a good reason to do that 😅 Would concurrency: true work? If so, we should use it, if not, we should set concurrency: false explicitly with a comment explaining why – but please treat this as a nit and feel free to ignore if you prefer

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

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.

ah okok I see, no I'm totally happy to add concurrency: true 🙂👍

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

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.

concurrency: true added (63034cb) 🙂

Also concurrency: true was actually causing some failures because the fact that I was using the same global variable foo in two different tests so I fixed that, this also helped me notice that the useGlobale: false test could be improved to make sure global variables are inherited by the REPL (which is a documented behavior: https://nodejs.org/api/repl.html#global-and-local-scope), thanks for that! 😄 🫶

Please have a look and let me know if things look good to you now

it('uses the custom eval function as expected', () => {
const output = getReplOutput('Convert this to upper case', {
Expand Down Expand Up @@ -108,25 +130,3 @@ describe('repl with custom eval', () => {
);
});
});

function getReplOutput(input, replOptions, run = true) {
const inputStream = new ArrayStream();
const outputStream = new ArrayStream();

repl.start({
input: inputStream,
output: outputStream,
...replOptions,
});

let output = '';
outputStream.write = (chunk) => (output += chunk);

inputStream.emit('data', input);

if (run) {
inputStream.run(['']);
}

return output;
}