test: add tests for REPL custom evals by dario-piotrowicz · Pull Request #57691 · nodejs/node · GitHub

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

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 }, () => {

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?

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

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? 🙂

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

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

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

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

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

Landed in 1f7cfb7

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

Another failure here https://ci.nodejs.org/job/node-test-commit-linux-containered/49958/nodes=ubuntu2204_sharedlibs_withoutssl_x64/testReport/(root)/parallel/test_repl_custom_eval/

---
duration_ms: 171.809
exitcode: 1
severity: fail
stack: |-
  > > > > Test failure: 'does show previews if `preview` is set to `true`'
  Location: test/parallel/test-repl-custom-eval.js:121:3
  AssertionError [ERR_ASSERTION]: The input did not match the regular expression /'Hello custom' \+ ' eval World!'\n\/\/ 'Hello custom eval World!'/. Input:

  "'Hello custom' + ' eval World!'"

      at TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-repl-custom-eval.js:130:12)
      at Test.runInAsyncScope (node:async_hooks:214:14)
      at Test.run (node:internal/test_runner/test:1056:25)
      at Test.start (node:internal/test_runner/test:953:17)
      at node:internal/test_runner/test:1452:71
      at node:internal/per_context/primordials:483:82
      at new Promise (<anonymous>)
      at new SafePromise (node:internal/per_context/primordials:451:29)
      at node:internal/per_context/primordials:483:9
      at Array.map (<anonymous>) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: "'Hello custom' + ' eval World!'",
    expected: /'Hello custom' \+ ' eval World!'\n\/\/ 'Hello custom eval World!'/,
    operator: 'match'
  }
...

I can't reproduce it locally.

@tniessen I'm so sorry for the inconvenience I've caused here 🙇

locally I could not see this problem and to be completely honest I don't pay a huge attention on CI as I often just assume that it has flakiness issues (besides these are pretty innocuous tests I'm surprised that they could cause issues 😓)

thanks for reverting it I will try to get it landed again if that's ok 🙂

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

this commit reintroduces the REPL custom eval tests that have been introduced in nodejs#57691 but reverted in nodejs#57793 the tests turned out problematic before because `getReplOutput`, the function used to return the repl output wasn't taking into account that input processing and output emitting are asynchronous operation can resolve with a small delay the new implementation here replaces `getReplOutput` with `getReplRunOutput` that resolves repl inputs by running them and using the repl prompt as an indicator to when the input processing has completed

this commit reintroduces the REPL custom eval tests that have been introduced in #57691 but reverted in #57793 the tests turned out problematic before because `getReplOutput`, the function used to return the repl output wasn't taking into account that input processing and output emitting are asynchronous operation can resolve with a small delay the new implementation here replaces `getReplOutput` with `getReplRunOutput` that resolves repl inputs by running them and using the repl prompt as an indicator to when the input processing has completed PR-URL: #57850 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

test: add tests for REPL custom evals

e8b6c18

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Mar 30, 2025

aduh95 reviewed Mar 31, 2025

View reviewed changes

Comment thread test/parallel/test-repl-custom-eval.js Outdated

aduh95 reviewed Mar 31, 2025

View reviewed changes

Comment thread test/parallel/test-repl-custom-eval.js Outdated

aduh95 reviewed Mar 31, 2025

View reviewed changes

Apply suggestions from code review

afd03ec

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

anonrig approved these changes Mar 31, 2025

View reviewed changes

move getReplOutput up

4da7e25

dario-piotrowicz requested a review from aduh95 April 1, 2025 10:14

bjohansebas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

github-actions Bot added the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 3, 2025

use concurrency: true and adjust tests

63034cb

aduh95 approved these changes Apr 5, 2025

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2025

This comment was marked as outdated.

Sign in to view

aduh95 merged commit 1f7cfb7 into nodejs:main Apr 7, 2025

dario-piotrowicz deleted the dario/test/repl-custom-eval branch April 7, 2025 22:29

tniessen mentioned this pull request Apr 8, 2025

Revert "test: add tests for REPL custom evals" #57793

Merged

RafaelGSS pushed a commit that referenced this pull request May 1, 2025

test: add tests for REPL custom evals

33b615b

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

RafaelGSS pushed a commit that referenced this pull request May 2, 2025

test: add tests for REPL custom evals

3cdf8ec

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 pushed a commit that referenced this pull request May 6, 2025

test: add tests for REPL custom evals

a6df4c6

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request May 14, 2025

test: add tests for REPL custom evals

66bd7c8

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 pushed a commit that referenced this pull request May 16, 2025

test: add tests for REPL custom evals

739c037

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 pushed a commit that referenced this pull request May 17, 2025

test: add tests for REPL custom evals

7b77815

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

github-actions Bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

aduh95 pushed a commit that referenced this pull request May 19, 2025

test: add tests for REPL custom evals

f3a4d03

PR-URL: #57691 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

ghost mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed


		const repl = require('repl');

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

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

test: add tests for REPL custom evals by dario-piotrowicz · Pull Request #57691 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: add tests for REPL custom evals - #57691

test: add tests for REPL custom evals#57691aduh95 merged 4 commits intonodejs:mainnodejs/node:mainfrom dario-piotrowicz:dario/test/repl-custom-evaldario-piotrowicz/node:dario/test/repl-custom-evalCopy head branch name to clipboard

Conversation

dario-piotrowicz commented Mar 30, 2025

Uh oh!

codecov Bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

aduh95 Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

aduh95 Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

aduh95 Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 3, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 6, 2025

Uh oh!

aduh95 commented Apr 7, 2025

Uh oh!

tniessen commented Apr 8, 2025

Uh oh!

lpinca commented Apr 8, 2025

Uh oh!

dario-piotrowicz commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test: add tests for REPL custom evals#57691
aduh95 merged 4 commits into
nodejs:mainfrom
dario-piotrowicz:dario/test/repl-custom-eval

codecov Bot commented Mar 30, 2025 •
edited
Loading

aduh95 Apr 5, 2025 •
edited
Loading