GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: add tests for REPL custom evals by dario-piotrowicz · Pull Request #57691 · nodejs/node

Conversation

@dario-piotrowicz

@dario-piotrowicz dario-piotrowicz commented Mar 30, 2025

Copy link
Copy Markdown
Member

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 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dario-piotrowicz
test: add tests for REPL custom evals
e8b6c18
@nodejs-github-bot 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
@codecov

codecov Bot commented Mar 30, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Mar 31, 2025
View reviewed changes
Comment thread test/parallel/test-repl-custom-eval.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Mar 31, 2025
View reviewed changes
Comment thread test/parallel/test-repl-custom-eval.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Mar 31, 2025
View reviewed changes
Comment thread test/parallel/test-repl-custom-eval.js Outdated

const repl = require('repl');

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

@aduh95 aduh95 Mar 31, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dario-piotrowicz dario-piotrowicz Mar 31, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Apr 4, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Apr 5, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dario-piotrowicz dario-piotrowicz Apr 5, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dario-piotrowicz @aduh95
Apply suggestions from code review
afd03ec 
Co-authored-by: Antoine du Hamel <[email protected]>
anonrig
anonrig approved these changes Mar 31, 2025
View reviewed changes
@dario-piotrowicz
move getReplOutput up
4da7e25
@dario-piotrowicz
dario-piotrowicz requested a review from aduh95 April 1, 2025 10:14
@bjohansebas bjohansebas added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 3, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 3, 2025
@github-actions

github-actions Bot commented Apr 3, 2025

Copy link
Copy Markdown
Contributor
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. label Apr 3, 2025
@dario-piotrowicz
use concurrency: true and adjust tests
63034cb
aduh95
aduh95 approved these changes Apr 5, 2025
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Apr 5, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 5, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 6, 2025

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 1f7cfb7 into nodejs:main Apr 7, 2025
@aduh95

aduh95 commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

Landed in 1f7cfb7
dario-piotrowicz reacted with heart emoji dario-piotrowicz reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dario-piotrowicz
dario-piotrowicz deleted the dario/test/repl-custom-eval branch April 7, 2025 22:29
@tniessen

tniessen commented Apr 8, 2025

Copy link
Copy Markdown
Member

@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.
lpinca and aduh95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen tniessen mentioned this pull request Apr 8, 2025
Revert "test: add tests for REPL custom evals" #57793
Merged
@lpinca

lpinca commented Apr 8, 2025

Copy link
Copy Markdown
Member

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.
dario-piotrowicz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dario-piotrowicz

dario-piotrowicz commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

@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 🙂
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@dario-piotrowicz @RafaelGSS
test: add tests for REPL custom evals
33b615b 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@RafaelGSS 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
@dario-piotrowicz @RafaelGSS
test: add tests for REPL custom evals
3cdf8ec 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
a6df4c6 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@dario-piotrowicz @RafaelGSS
test: add tests for REPL custom evals
66bd7c8 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
739c037 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
7b77815 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@github-actions 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
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
f3a4d03 
PR-URL: #57691
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@ghost ghost mentioned this pull request Jun 8, 2025
### Notable Changes autofix-ci/action#28
Closed
dario-piotrowicz added a commit to dario-piotrowicz/node that referenced this pull request Jun 11, 2025
@dario-piotrowicz
test: add tests for REPL custom evals
a5ef49b 
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
nodejs-github-bot pushed a commit that referenced this pull request Jun 15, 2025
@dario-piotrowicz
test: add tests for REPL custom evals
d6dade5 
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Jun 16, 2025
@dario-piotrowicz @targos
test: add tests for REPL custom evals
704b1fa 
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 pushed a commit that referenced this pull request Jul 21, 2025
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
ad8c6fc 
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 pushed a commit that referenced this pull request Jul 24, 2025
@dario-piotrowicz @aduh95
test: add tests for REPL custom evals
9d3e451 
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@richeshgupta richeshgupta mentioned this pull request Sep 23, 2026
AACR nodejs__node@e8b6c18 razorpaytest/node#1
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@dario-piotrowicz @nodejs-github-bot @aduh95 @tniessen @lpinca @anonrig @bjohansebas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.