Skip to content

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

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
Merged
nodejs-github-bot merged 6 commits into from
Apr 3, 2022
Merged

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

nodejs-github-bot merged 6 commits into from
Apr 3, 2022

Conversation

@fossamagna
Copy link
Contributor

@fossamagnafossamagna commented Mar 21, 2022

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-botnodejs-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

@aduh95aduh95 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

@fossamagna
Copy link
ContributorAuthor

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.

@fossamagnafossamagna requested a review from aduh95March 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
ContributorAuthor

fossamagna commented Mar 24, 2022

@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
@aduh95aduh95 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-actionsgithub-actionsbot 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

nodejs-github-bot commented Mar 26, 2022

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

nodejs-github-bot commented Apr 2, 2022

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

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

nodejs-github-bot commented Apr 3, 2022

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

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

nodejs-github-bot commented Apr 3, 2022

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@github-actionsgithub-actionsbot 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@juanarbol
Copy link
Member

juanarbol commented May 30, 2022

Dependent of #37947

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

Reviewers

@jasnelljasnelljasnell approved these changes

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fossamagna@aduh95@nodejs-github-bot@juanarbol@jasnell