Skip to content

test: refactor repl save-load tests - #58715

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
dario-piotrowicz:dario/refactor-test-repl-save-load
Jun 22, 2025
Merged

test: refactor repl save-load tests#58715
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
dario-piotrowicz:dario/refactor-test-repl-save-load

Conversation

@dario-piotrowicz

@dario-piotrowiczdario-piotrowicz commented Jun 15, 2025

Copy link
Copy Markdown
Member

refactor the test/parallel/test-repl-save-load.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)
  • clearly separating and commenting the various tests to make clearer what is being tested

Pretty much the same as #58636 🙂

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 15, 2025
@codecov

codecovBot commented Jun 15, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.11%. Comparing base (5584cc5) to head (e5fcd0f).
Report is 180 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #58715 +/- ##
==========================================
- Coverage 90.13% 90.11% -0.02% 
==========================================
Files 639 639 Lines 188192 188192 Branches 36916 36905 -11 ==========================================
- Hits 169633 169598 -35 - Misses 11324 11339 +15 - Partials 7235 7255 +20 

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

@lpinca

lpinca commented Jun 15, 2025

Copy link
Copy Markdown
Member

using the test runner with appropriate descriptions to make clearer what is being tested

It was discussed ad ad nauseam and there is no real benefit to these refactorings. Can you please add comments instead or split the tests into multiple files?

@dario-piotrowicz

Copy link
Copy Markdown
MemberAuthor

using the test runner with appropriate descriptions to make clearer what is being tested

It was discussed ad ad nauseam and there is no real benefit to these refactorings. Can you please add comments instead or split the tests into multiple files?

The tests needed refactoring and since I was already at it I didn't see much harm in going with the test runner (which is my personal preference).

That is not the focal point of this refactoring so I will refactor this not to use the test runner not to get into already overly discussed arguments.

@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/refactor-test-repl-save-load branch from aca8dd8 to 54657a5CompareJune 15, 2025 20:23
@dario-piotrowicz

Copy link
Copy Markdown
MemberAuthor

test runner usage removed

@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/refactor-test-repl-save-load branch from 54657a5 to e6ff309CompareJune 15, 2025 20:26
Comment threadtest/parallel/test-repl-save-load.js Outdated
Comment threadtest/parallel/test-repl-save-load.js Outdated
refactor the test/parallel/test-repl-save-load.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)
- clearly separating and commenting the various tests to make
clearer what is being tested
@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/refactor-test-repl-save-load branch from e6ff309 to 97e90bfCompareJune 17, 2025 22:01
split tests over multiple files
@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

@dario-piotrowicz

Copy link
Copy Markdown
MemberAuthor

thanks @lpinca 🫶

@dario-piotrowiczdario-piotrowicz added 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. labels Jun 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@dario-piotrowiczdario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2025
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 21, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/58715
✔ Done loading data for nodejs/node/pull/58715
----------------------------------- PR info ------------------------------------
Title test: refactor repl save-load tests (#58715)
Author Dario Piotrowicz <dario.piotrowicz@gmail.com> (@dario-piotrowicz)
Branch dario-piotrowicz:dario/refactor-test-repl-save-load -> nodejs:main
Labels test, author ready, needs-ci, commit-queue-squash
Commits 2
- test: refactor repl save-load tests
- fixup! test: refactor repl save-load tests
Committers 1
- Dario Piotrowicz <dario.piotrowicz@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/58715
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58715
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
--------------------------------------------------------------------------------
ℹ This PR was created on Sun, 15 Jun 2025 12:55:54 GMT
✔ Approvals: 1
✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/58715#pullrequestreview-2943994619
✘ This PR needs to wait 24 more hours to land (or 0 hours if there is one more approval)
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2025-06-20T17:08:02Z: https://ci.nodejs.org/job/node-test-pull-request/67573/
- Querying data for job/node-test-pull-request/67573/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/15795695104

@dario-piotrowiczdario-piotrowicz removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 21, 2025
@dario-piotrowiczdario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 910a8af into nodejs:mainJun 22, 2025
12 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 910a8af

@dario-piotrowicz
dario-piotrowicz deleted the dario/refactor-test-repl-save-load branch June 22, 2025 18:26
RafaelGSS pushed a commit that referenced this pull request Jun 23, 2025
refactor the test/parallel/test-repl-save-load.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)
- clearly separating and commenting the various tests to make
clearer what is being tested
PR-URL: #58715
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jul 21, 2025
@aduh95

Copy link
Copy Markdown
Contributor

Backport is blocked on the backport of #57909

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@dario-piotrowicz@lpinca@nodejs-github-bot@aduh95