FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add tests for REPL custom evals (second attempt) by dario-piotrowicz · Pull Request #57850 · nodejs/node · GitHub

/ node Public

test: add tests for REPL custom evals (second attempt) - #57850

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
dario-piotrowicz:dario/test/repl-custom-eval-2
Jun 15, 2025
ConversationCommits (1)ChecksFiles changed
Merged

test: add tests for REPL custom evals (second attempt)#57850
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
dario-piotrowicz:dario/test/repl-custom-eval-2

Conversation

dario-piotrowicz commented Apr 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

this PR reintroduces the REPL custom eval tests that have been introduced in #57691 but reverted in #57793

the tests were working fine locally but caused failures in CI (like this one) the problem, I believe, being that the getReplOutput would return the output value right away even though the repl processing is asynchronous. Some small delay there is, I imagine, present in the CI runs and that's what I think was causing the issue.

I addressed this problem by returning a promise that resolves to the repl's output using the repl prompt as the signal to understand when the input processing has concluded, instead of returning the output right away.

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

dario-piotrowicz commented Apr 12, 2025

Copy link
Copy Markdown
Member Author

cc. @tniessen, @lpinca 🙂 (sorry again for my earlier mistake on this change 🙇)

dario-piotrowicz force-pushed the dario/test/repl-custom-eval-2 branch from 8c394e5 to 9413135 Compare April 12, 2025 17:12
dario-piotrowicz changed the title test: add tests for REPL custom evals test: add tests for REPL custom evals (second attempt) Apr 12, 2025
dario-piotrowicz force-pushed the dario/test/repl-custom-eval-2 branch from 9413135 to 1e33c67 Compare April 12, 2025 17:14

codecov Bot commented Apr 12, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.16%. Comparing base (58e1cba) to head (a5ef49b).
Report is 34 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57850      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.01%     
==========================================
  Files         636      637       +1     
  Lines      188028   188122      +94     
  Branches    36895    36908      +13     
==========================================
+ Hits       169547   169622      +75     
- Misses      11233    11237       +4     
- Partials     7248     7263      +15
Files with missing lines Coverage Δ
lib/repl.js 95.12% <ø> (-0.01%) ⬇️

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

BridgeAR approved these changes Apr 14, 2025
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

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

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

lpinca commented Apr 16, 2025

Copy link
Copy Markdown
Member

There are still failures. Here is one in the latest CI run: https://ci.nodejs.org/job/node-test-commit-linux-containered/50117/nodes=ubuntu2204_sharedlibs_withoutssl_x64/testReport/(root)/parallel/test_repl_custom_eval/

---
duration_ms: 826.053
exitcode: 1
severity: fail
stack: |-
  > > > > Test failure: 'does show previews if `preview` is set to `true`'
  Location: test/parallel/test-repl-custom-eval.js:146: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:159:12)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:1069:7)
      at async Promise.all (index 7)
      at async Suite.run (node:internal/test_runner/test:1461:7)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:308:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: "'Hello custom' + ' eval World!'",
    expected: /'Hello custom' \+ ' eval World!'\n\/\/ 'Hello custom eval World!'/,
    operator: 'match'
  }
...

dario-piotrowicz commented Apr 17, 2025

Copy link
Copy Markdown
Member Author

oof... 😓

sorry about that, thanks @lpinca, I'll have a look 🙏

dario-piotrowicz force-pushed the dario/test/repl-custom-eval-2 branch from 1e33c67 to 584b6d9 Compare April 18, 2025 08:44

dario-piotrowicz commented Apr 18, 2025

Copy link
Copy Markdown
Member Author

@lpinca I couldn't really reproduce the issue locally but I refactored the code to make it more robust regarding timing, I hope that this will fix the CI check, could you re-run the tests? 🙏

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2025
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 18, 2025

github-actions Bot commented Apr 18, 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
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14532544242

nodejs-github-bot commented Apr 18, 2025

Copy link
Copy Markdown
Collaborator

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

lpinca removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 18, 2025
dario-piotrowicz added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2025
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 5, 2025

github-actions Bot commented May 5, 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
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14833744376

nodejs-github-bot commented May 5, 2025

Copy link
Copy Markdown
Collaborator

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

dario-piotrowicz force-pushed the dario/test/repl-custom-eval-2 branch from 584b6d9 to 8196294 Compare May 11, 2025 20:19
dario-piotrowicz force-pushed the dario/test/repl-custom-eval-2 branch from 9bd4725 to a5ef49b Compare June 11, 2025 11:51

nodejs-github-bot commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator

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

dario-piotrowicz marked this pull request as ready for review June 11, 2025 16:24

dario-piotrowicz commented Jun 11, 2025

Copy link
Copy Markdown
Member Author

@jasnell, @BridgeAR and @gurgunday I've significantly changed the code since your last approval (for the better, the tests now are very much faster and more reliable than my previous iterations), could you please give this one a re-review when you get the chance? 🙏

dario-piotrowicz removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Jun 11, 2025
dario-piotrowicz added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 13, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 15, 2025

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Jun 15, 2025

Copy link
Copy Markdown
Collaborator

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

dario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 15, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 15, 2025
nodejs-github-bot merged commit d6dade5 into nodejs:main Jun 15, 2025

nodejs-github-bot commented Jun 15, 2025

Copy link
Copy Markdown
Collaborator

Landed in d6dade5

dario-piotrowicz deleted the dario/test/repl-custom-eval-2 branch June 15, 2025 16:36
targos pushed a commit that referenced this pull request Jun 16, 2025
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
github-actions Bot mentioned this pull request Jun 16, 2025
Closed
This was referenced Jun 23, 2025
Closed
Merged
aduh95 pushed a commit that referenced this pull request Jul 21, 2025
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
aduh95 pushed a commit that referenced this pull request Jul 24, 2025
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
github-actions Bot mentioned this pull request Jul 28, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
BridgeAR Awaiting requested review from BridgeAR
gurgunday Awaiting requested review from gurgunday

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

6 participants

Back | FazBrowse Home | New Git URL