Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: improve lib/internal/readline/promises.js coverage #42420

Merged
merged 6 commits into from
Apr 3, 2022
Merged

test: improve lib/internal/readline/promises.js coverage #42420

merged 6 commits into from
Apr 3, 2022

Conversation

fossamagna
Copy link
Contributor

This improves a test coverage in lib/internal/readline/promises.js

ref: https://coverage.nodejs.org/coverage-419f02ba1f00cac3/lib/internal/readline/promises.js.html

This validates that autoCommit option work correctly.

@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 Mar 21, 2022
aduh95
aduh95 reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I think we can remove the test functions

test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
@fossamagna
Copy link
Contributor Author

fossamagna commented Mar 22, 2022 •
edited
Loading

@aduh95
Thank you for your review.

I removed test function in response to take your suggestion, but test got following error.

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ '\x1B[1K\x1B[0K'
- '\x1B[0K'
        ^

When use autoCommit option, readline write to stream in callback function of process.nextTick. If we removed test function, before readline write to stream and test assertion, invocation of clearLine function in for-of loop will be finished.

@fossamagna fossamagna requested a review from aduh95 March 23, 2022 13:06
@aduh95
Copy link
Contributor

aduh95 commented Mar 23, 2022

@fossamagna I suggest using await setImmediate from timers/promises instead of a test function + process.nextTick, that should solve the problem and hopefully be more readable. Let me know if that works for you.

@fossamagna
test: make test more readable
b215d31 
using `await setImmediate` instead of `test` function + process.nextTick.
@fossamagna
Copy link
Contributor Author

@aduh95 Thank you very much for your suggestion.
It works like a charm and have been more readable.

aduh95
aduh95 reviewed Mar 24, 2022
View reviewed changes
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
test/parallel/test-readline-promises-csi.mjs Outdated Show resolved Hide resolved
fossamagna and others added 2 commits March 24, 2022 18:41
@fossamagna @aduh95
Apply suggestions from code review
5c980e5 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@fossamagna
test: fix lint error
9630466
@aduh95 aduh95 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2022
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Mar 27, 2022
Open
Open
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Apr 3, 2022
Open
8 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 95f94cf into nodejs:master Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 95f94cf

juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@fossamagna @juanarbol
test: improve lib/internal/readline/promises.js coverage
44ac5e8 
PR-URL: nodejs#42420
Refs: https://coverage.nodejs.org/coverage-419f02ba1f00cac3/lib/internal/readline/promises.js.html
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 5, 2022
Closed
Merged
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
@fossamagna @juanarbol
test: improve lib/internal/readline/promises.js coverage
fb25ba4 
PR-URL: #42420
Refs: https://coverage.nodejs.org/coverage-419f02ba1f00cac3/lib/internal/readline/promises.js.html
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions bot mentioned this pull request Apr 7, 2022
Open
17 tasks
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@fossamagna @xtx1130
test: improve lib/internal/readline/promises.js coverage
db95fc1 
PR-URL: nodejs#42420
Refs: https://coverage.nodejs.org/coverage-419f02ba1f00cac3/lib/internal/readline/promises.js.html
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol
Copy link
Member

Dependent of #37947

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

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fossamagna @aduh95 @nodejs-github-bot @juanarbol @jasnell