Skip to content

Conversation

@dario-piotrowicz
Copy link
Member

@dario-piotrowiczdario-piotrowicz commented Jun 15, 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

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
Copy link

codecovbot commented Jun 15, 2025

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
Copy link
Member

lpinca commented Jun 15, 2025

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
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-piotrowiczdario-piotrowiczforce-pushed the dario/refactor-test-repl-save-load branch from aca8dd8 to 54657a5CompareJune 15, 2025 20:23
@dario-piotrowicz
Copy link
MemberAuthor

test runner usage removed

@dario-piotrowiczdario-piotrowiczforce-pushed the dario/refactor-test-repl-save-load branch from 54657a5 to e6ff309CompareJune 15, 2025 20:26
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-piotrowiczdario-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
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
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
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 <[email protected]> (@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 <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58715 Reviewed-By: Luigi Pinca <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58715 Reviewed-By: Luigi Pinca <[email protected]> -------------------------------------------------------------------------------- ℹ 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-botnodejs-github-bot merged commit 910a8af into nodejs:mainJun 22, 2025
76 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 910a8af

@dario-piotrowiczdario-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 <[email protected]>
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jul 21, 2025
@aduh95
Copy link
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