Skip to content

test: add test to improve coverage in test-http2-compat-serverresponse-write.js #44970

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

Cesar-M-Diaz
Copy link
Contributor

Added a test to complement the test coverage on this file test-http2-compat-serverresponse-write.js

@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 Oct 11, 2022
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

request.resume();
request.on('end', mustCall());
request.on('close', mustCall(() => {
client.close();
Copy link
Member

@lpinca lpinca Oct 11, 2022

Choose a reason for hiding this comment

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

Are both this and client.destroy(); below needed? I would keep only one if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is not necessary, just pushed a new commit for that. Thanks

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2022
@nodejs-github-bot
Copy link
Collaborator

@juanarbol
Copy link
Member

CI failing is not related to this change. Requesting a CI run again

@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

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

LGTM

@Cesar-M-Diaz
Copy link
Contributor Author

@lpinca PTAL

@lpinca
Copy link
Member

lpinca commented Oct 14, 2022

@Cesar-M-Diaz it seems good to me but I'm not sure I understand what is tested in particular. Can you please add a comment or clarify how coverage is increased with a link or an image? Thanks.

@Cesar-M-Diaz
Copy link
Contributor Author

@lpinca Im covering this statement with the test
image

@lpinca
Copy link
Member

lpinca commented Oct 14, 2022

Thanks.

@juanarbol juanarbol added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 14, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 14, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4264b19 into nodejs:main Oct 14, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 4264b19

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
PR-URL: #44970
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
PR-URL: #44970
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #44970
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #44970
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #44970
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants