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 code coverage for string decoder #22306

Closed
wants to merge 2 commits into from
Closed

test: improve code coverage for string decoder #22306

wants to merge 2 commits into from

Conversation

BeniCheni
Copy link
Contributor

This PR attempts to cover this line of uncovered string decoder code from the coverage report, after exploring with nodejs/getting-started repo that led to consulting with nodetodo.org about contributing to unit test code.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 14, 2018
@trivikr
Copy link
Member

trivikr commented Aug 14, 2018

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

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2018
@Trott
Copy link
Member

Trott commented Aug 14, 2018 •
edited by BridgeAR
Loading

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16452/ ✔️

@joyeecheung joyeecheung mentioned this pull request Aug 15, 2018
Closed
@BeniCheni
Copy link
Contributor Author

BeniCheni commented Aug 24, 2018 •
edited
Loading

Hi, collaborators, (cc @trivikr @Trott who helped to post CI instances in previous comments)

Saw some approvals & read the previous failed CI instance w. the referred issue above.

I have rebased the feature branch of this PR and pushed, as an attempt to obtain a "green" CI instance. After the attempt, the current PR seems to have a "green" CI instance.

Would you mind guiding this PR to "land", after the "green" CI instance has been obtained & approvals?

@addaleax
Copy link
Member

Landed in 5673017 🎉

@BeniCheni Thanks! Usually a simple ping is enough to get things done.

I have rebased the feature branch of this PR and pushed

By the way, it looks like you used something that creates a “merge commit” (which is what you get by default from running git pull or git merge), not a typical rebase.

@addaleax addaleax closed this Aug 24, 2018
addaleax pushed a commit that referenced this pull request Aug 24, 2018
@BeniCheni @addaleax
test: improve code coverage for string decoder
5673017 
PR-URL: #22306
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BeniCheni
Copy link
Contributor Author

Thank you for landing! Good to learn about the rebase workflow, which I'd be mindful about.

@BeniCheni BeniCheni deleted the string-decoder-test-coverage branch August 24, 2018 18:07
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@BeniCheni @addaleax
test: improve code coverage for string decoder
271636a 
PR-URL: #22306
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@BeniCheni @targos
test: improve code coverage for string decoder
9ec105c 
PR-URL: #22306
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@BeniCheni @targos
test: improve code coverage for string decoder
a704ed8 
PR-URL: #22306
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca 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. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BeniCheni @trivikr @Trott @addaleax @jasnell @lpinca @cjihrig @BridgeAR @nodejs-github-bot