Skip to content

Conversation

dario-piotrowicz
Copy link
Member

This PR adds a new testing utility called startNewREPLSever which simply starts a new repl server that can be used for testing.

The reasoning for proposing this change are to reduce unnecessary code repetition (notice that the PR is net removing more than 450 lines of code1) and to also provide some consistency on how repl server are tested.

Currently in tests when a new repl server gets started some input/output streams need to be passed to it, sometimes they are implemented via new stream.PassThrough(), some other times (more commonly) via new ArrayStream(), sometimes a single stream is passed both as the input and output, sometimes different streams are passed instead.
In most cases (as far as I can tell) there isn't a specific reason as to why an implementation has been chosen or not. startNewREPLSever removes this variation by adopting what looked to me like the most commonly used implementation aspects.

Another nice thing is that startNewREPLSever helps with variable name consistency. Currently when a test repl server is started it is stored in a variable which name can be r, replServer, server or testMe. Input streams can be input, putIn or inputStream. Output streams can be output, putIn or outputStream. (I am likely forgetting some variation too). startNewREPLSever consistently refers to these values respectively as replServer , input and output.


Please note that my PR is not replacing all repl server starts in tests with startNewREPLSever, mainly because sometimes that is not helpful (for example when testing the repl constructor default values) or because it would require some involved refactoring (and I think that this PR is pretty big as it is 😅, if this lands I can also see as a followup if startNewREPLSever could, be used in a few other tests as well)

Footnotes

  1. The output accumulation logic is re-implemented multiple times as well as the error domain on error handler logic.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 21, 2025
@dario-piotrowicz dario-piotrowicz added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2025
@github-actions 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 Sep 21, 2025

This comment was marked as outdated.

Copy link

codecov bot commented Sep 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.41%. Comparing base (3625623) to head (9112a5c).
⚠️ Report is 46 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59964      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         703      703              
  Lines      207398   207419      +21     
  Branches    39987    39997      +10     
==========================================
+ Hits       183374   183390      +16     
- Misses      15995    15998       +3     
- Partials     8029     8031       +2     

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

@dario-piotrowicz dario-piotrowicz removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Sep 21, 2025
@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 23, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2025
@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Sep 30, 2025
PR-URL: #59964
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@addaleax
Copy link
Member

Landed in 9ac571d

@addaleax addaleax closed this Sep 30, 2025
@dario-piotrowicz dario-piotrowicz deleted the dario/test/common/startNewREPLServer branch October 4, 2025 16:10
targos pushed a commit that referenced this pull request Oct 6, 2025
PR-URL: #59964
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants