test: add tests for REPL custom evals

[dario-piotrowicz]

I noticed this TODO comment mentioning that some tests were needed for custom REPL evals so I figured I could add some 🙂

I had a double check and I don't think that the functionality I am testing here is already tested in other test files 🙂

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (af75d04) to head (63034cb).
Report is 79 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57691      +/-   ##
==========================================
+ Coverage   90.23%   90.25%   +0.01%     
==========================================
  Files         630      630              
  Lines      185055   185203     +148     
  Branches    36221    36293      +72     
==========================================
+ Hits       166984   167152     +168     
+ Misses      11043    11002      -41     
- Partials     7028     7049      +21     

Files with missing lines	Coverage Δ
lib/repl.js	94.91% <ø> (+0.01%)	⬆️

... and 59 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

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]

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]

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]

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]

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]

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

[dario-piotrowicz]

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

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - test: add tests for REPL custom evals
   ⚠  - Apply suggestions from code review
   ⚠  - move `getReplOutput` up
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14252784092

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66076/

[aduh95]

Landed in 1f7cfb7

[tniessen]

@dario-piotrowicz @anonrig @aduh95 This test appears to fail in other PRs now (e.g., in every single CI run on #57787).

The test also appears to have failed in this PR but that comment was hidden. I might be missing something but I don't think this PR should have landed.