Skip to content

repl: fix multiline history editing string order - #57874

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
puskin:repl-edit-multiline-reverse-strig
Apr 16, 2025
Merged

repl: fix multiline history editing string order#57874
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
puskin:repl-edit-multiline-reverse-strig

Conversation

@puskin

@puskinpuskin commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

fixing an issue introduced in the last commit of #57400:
when editing a multiline history in REPL, the string gets reversed.

Added relevant automated tests

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Apr 14, 2025
BridgeAR
BridgeAR reviewed Apr 14, 2025
View reviewed changes
Comment threadlib/internal/readline/interface.js Outdated
BridgeAR
BridgeAR reviewed Apr 14, 2025
View reviewed changes

@BridgeARBridgeAR 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.

Would this also fix the flaky behavior reported in the comment? :)

#57400 (comment)

@codecov

codecovBot commented Apr 14, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (f89baf2) to head (03cd310).
Report is 297 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57874 +/- ##
==========================================
- Coverage 90.24% 90.24% -0.01% 
==========================================
Files 630 630 Lines 185670 185694 +24 Branches 36401 36404 +3 ==========================================
+ Hits 167567 167577 +10 - Misses 10992 10999 +7 - Partials 7111 7118 +7
Files with missing linesCoverage Δ
lib/internal/readline/interface.js96.79% <100.00%> (+0.01%)⬆️

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

@puskin
puskinforce-pushed the repl-edit-multiline-reverse-strig branch from 50800d0 to 39b827cCompareApril 14, 2025 16:05
@puskin

puskin commented Apr 14, 2025

Copy link
Copy Markdown
ContributorAuthor

Would this also fix the flaky behavior reported in the comment? :)

#57400 (comment)

I just added some code which could fix the flakiness... I am not sure if it is good / clean enough tho, I didn't have any cooler idea 😞

BridgeAR
BridgeAR reviewed Apr 14, 2025
View reviewed changes

@BridgeARBridgeAR 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.

I don't think the wait should make a difference for the test being flaky. I could imagine we run tests in parallel that use the same repl file to write to and they conflict with each other. Or is that unique to the current tmp directory?

The flaky result said we had two history entries.

@puskin

puskin commented Apr 14, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Yeah I interpreted the two history entries as in a weird combination of keypresses which were not properly "awaited" for. Unfortunately I cannot reproduce the issue, let me investigate the "multiple access to the same history file" path

EDIT: applied your suggestion, let's see if it happens again!

@puskin
puskinforce-pushed the repl-edit-multiline-reverse-strig branch from 39b827c to 4a9c482CompareApril 14, 2025 17:35
@BridgeARBridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator

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

@puskinpuskin mentioned this pull request Apr 15, 2025
Merged
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

lpinca
lpinca reviewed Apr 15, 2025
View reviewed changes
Comment threadtest/parallel/test-repl-multiline-navigation.js Outdated
Comment threadtest/parallel/test-repl-multiline-navigation.js Outdated
@puskin
puskinforce-pushed the repl-edit-multiline-reverse-strig branch from 4a9c482 to 84f0885CompareApril 15, 2025 08:57
@puskin
puskinforce-pushed the repl-edit-multiline-reverse-strig branch from 84f0885 to 03cd310CompareApril 15, 2025 09:02
@BridgeARBridgeAR added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 15, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator

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

@pmarchinipmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-bot
nodejs-github-bot merged commit af85f3f into nodejs:mainApr 16, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator

Landed in af85f3f

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
Giovanni Bucci@RafaelGSS
repl: fix multiline history editing string order
b1df3e9
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
Giovanni Bucci@RafaelGSS
repl: fix multiline history editing string order
ab99ee6
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
Giovanni Bucci@aduh95
repl: fix multiline history editing string order
fc75cba
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
Giovanni Bucci@aduh95
repl: fix multiline history editing string order
a85647a
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
Giovanni Bucci@RafaelGSS
repl: fix multiline history editing string order
cfe5036
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
Giovanni Bucci@aduh95
repl: fix multiline history editing string order
037b2d8
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
Giovanni Bucci@aduh95
repl: fix multiline history editing string order
de954dc
PR-URL: #57874
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 May 18, 2025
@aduh95

aduh95 commented May 18, 2025

Copy link
Copy Markdown
Contributor

test-repl-multiline-navigation is failing on Windows with this change and on v22.x-staging, it would require a manual backport if we want it on 22.x

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

Reviewers

@lpincalpincalpinca approved these changes
@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

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.needs-ciPRs that need a full CI run.readlineIssues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@puskin@nodejs-github-bot@aduh95@lpinca@BridgeAR@pmarchini