Skip to content

test: refactor repl tab complete tests #58636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Jun 8, 2025

refactor the test/parallel/test-repl-tab-complete.js file by:

  • making the tests in the file self-contained (instead of all of them sharing the same REPL instance and constantly calling .clear on it)
  • using the test runner with appropriate descriptions to make clearer what is being tested
  • extracting some tests in their own js test files (to increase isolation of the tests and help with issues such as flakiness)

@dario-piotrowicz dario-piotrowicz requested a review from BridgeAR June 8, 2025 21:49
@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 Jun 8, 2025
Comment on lines -56 to -61
// TODO: the following async IIFE and the completePromise function are necessary because
// the reply tests are all run against the same repl instance (testMe) and thus coordination
// needs to be in place for the tests not to interfere with each other, this is really
// not ideal, the tests in this file should be refactored so that each use its own isolated
// repl instance, making sure that no special coordination needs to be in place for them
// and also allowing the tests to all be run in parallel
Copy link
Member Author

Choose a reason for hiding this comment

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

PS: this is the driving reason for this PR

Copy link

codecov bot commented Jun 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.15%. Comparing base (5fe7800) to head (0d6cb84).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58636      +/-   ##
==========================================
- Coverage   90.16%   90.15%   -0.02%     
==========================================
  Files         637      637              
  Lines      188001   188001              
  Branches    36881    36877       -4     
==========================================
- Hits       169509   169484      -25     
- Misses      11238    11286      +48     
+ Partials     7254     7231      -23     

see 46 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 force-pushed the dario/repl-complete-tests-refactor branch from ae1c33f to ce45b2a Compare June 9, 2025 09:05
@dario-piotrowicz dario-piotrowicz added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 11, 2025
@dario-piotrowicz
Copy link
Member Author

dario-piotrowicz commented Jun 14, 2025

cc. @nodejs/repl would anyone like to have a look? 🙂

dario-piotrowicz and others added 2 commits June 14, 2025 18:17
refactor the test/parallel/test-repl-tab-complete.js file by:
 - making the tests in the file self-contained
   (instead of all of them sharing the same REPL instance and
   constantly calling `.clear` on it)
 - using the test runner with appropriate descriptions to make
   clearer what is being tested
 - extracting some tests in their own js test files
   (to increase isolation of the tests and help with issues such
   as flakiness)
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@dario-piotrowicz dario-piotrowicz force-pushed the dario/repl-complete-tests-refactor branch from 40bfac8 to 0d6cb84 Compare June 14, 2025 17:17
@dario-piotrowicz dario-piotrowicz added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2025
@nodejs-github-bot

This comment was marked as outdated.

@dario-piotrowicz dario-piotrowicz added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jun 15, 2025
@nodejs-github-bot
Copy link
Collaborator

@dario-piotrowicz dario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 15, 2025
@nodejs-github-bot 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 nodejs-github-bot merged commit aee9bc0 into nodejs:main Jun 15, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in aee9bc0

@dario-piotrowicz dario-piotrowicz deleted the dario/repl-complete-tests-refactor branch June 15, 2025 10:10
targos pushed a commit that referenced this pull request Jun 16, 2025
refactor the test/parallel/test-repl-tab-complete.js file by:
 - making the tests in the file self-contained
   (instead of all of them sharing the same REPL instance and
   constantly calling `.clear` on it)
 - using the test runner with appropriate descriptions to make
   clearer what is being tested
 - extracting some tests in their own js test files
   (to increase isolation of the tests and help with issues such
   as flakiness)

PR-URL: #58636
Reviewed-By: Giovanni Bucci <github@puskin.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

5 participants