FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: improve code coverage for streams/duplexify by ErickWendel · Pull Request #41862 · nodejs/node · GitHub

/ node Public

test: improve code coverage for streams/duplexify - #41862

Merged
nodejs-github-bot merged 4 commits into
nodejs:masterfrom
ErickWendel:test/streams-improve-code-coverage
Feb 11, 2022
ConversationCommits (4)ChecksFiles changed
Merged

test: improve code coverage for streams/duplexify#41862
nodejs-github-bot merged 4 commits into
nodejs:masterfrom
ErickWendel:test/streams-improve-code-coverage

Conversation

ErickWendel commented Feb 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Improve code coverage for streams duplexify.js

Refs:

I would add that those lines are unreachable:

  • duplexify.js.html#L71: We already validated all types of streams it'll never call the isNodeStream function
  • duplexify.js.html#L199: all data types were checked on the previous statements. The only values that could throw here would be primitive values but the assert won't let it run.
  • duplexify.js.html#L161: it'll never be undefined as the statement checks if typeof body?.readable === 'object'
  • duplexify.js.html#L166: it'll never be undefined as the statement checks if typeof body?.writable === 'object'

In those cases, what could I do to cover it?

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Feb 5, 2022

Trott commented Feb 5, 2022

Copy link
Copy Markdown
Member

If we have to use --expose-internals then that's fine, but it would be preferable if there was a way to cover the lines using public APIs.

Trott commented Feb 5, 2022

Copy link
Copy Markdown
Member

If we have to use --expose-internals then that's fine, but it would be preferable if there was a way to cover the lines using public APIs.

(We can also land this and try to refactor things later to use public APIs where possible.)

Trott commented Feb 5, 2022

Copy link
Copy Markdown
Member

In those cases, what could I do to cover it?

If could is unreachable, you can delete it. If you want to be cautious, you can replace it with an internal assertion. (https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/lib/internal/cluster/shared_handle.js is an example of something that uses the internal assertion.)

test: make assertions on duplex.from instead of internal.duplexify
38c8b4e 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>

ErickWendel commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

If we have to use --expose-internals then that's fine, but it would be preferable if there was a way to cover the lines using public APIs.

Just updated it using Public APIs

ErickWendel commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

In those cases, what could I do to cover it?

If could is unreachable, you can delete it. If you want to be cautious, you can replace it with an internal assertion. (https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/lib/internal/cluster/shared_handle.js is an example of something that uses the internal assertion.)

Perfect, I'll create another PR to land this, ok?

Mesteery added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2022

nodejs-github-bot commented Feb 7, 2022

Copy link
Copy Markdown
Collaborator

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

cjihrig reviewed Feb 7, 2022
View reviewed changes
Comment thread test/parallel/test-stream-duplex-from.js Outdated
github-actions Bot mentioned this pull request Feb 8, 2022
Open
22 tasks
test: add strictEqual for primitive values
1530f06 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
test: add strictEqual to ensure objects references
3e3a3ed 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
github-actions Bot mentioned this pull request Feb 9, 2022
Open
24 tasks
github-actions Bot mentioned this pull request Feb 10, 2022
Open
25 tasks
Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2022

nodejs-github-bot commented Feb 10, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Feb 11, 2022
Open
22 tasks
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 11, 2022
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2022
nodejs-github-bot merged commit 0185464 into nodejs:master Feb 11, 2022

nodejs-github-bot commented Feb 11, 2022

Copy link
Copy Markdown
Collaborator

Landed in 0185464

This was referenced Feb 12, 2022
Open
Open
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
test: improve code coverage for streams/duplexify
f4304ea 
PR-URL: nodejs#41862
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
test: improve code coverage for streams/duplexify
d49155b 
PR-URL: nodejs#41862
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bengl mentioned this pull request Feb 21, 2022
Merged
bengl pushed a commit that referenced this pull request Feb 22, 2022
test: improve code coverage for streams/duplexify
83b913c 
PR-URL: #41862
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

danielleadams commented Apr 19, 2022

Copy link
Copy Markdown
Contributor

@ErickWendel this breaks tests when landing in v16.x-staging. Do you mind creating a backport PR for the v16.x line? Thank you

ErickWendel commented Apr 19, 2022

Copy link
Copy Markdown
Member Author

@ErickWendel this breaks tests when landing in v16.x-staging. Do you mind creating a backport PR for the v16.x line? Thank you

Heyy. Sure! Do you have an example of how to do it? I'm not sure if it's just to make it works on the v16.x

ErickWendel mentioned this pull request Apr 19, 2022
Closed
targos pushed a commit that referenced this pull request Jul 12, 2022
test: improve code coverage for streams/duplexify
4aa4f7c 
PR-URL: #41862
Backport-PR-URL: #42788
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
test: improve code coverage for streams/duplexify
147f99c 
PR-URL: #41862
Backport-PR-URL: #42788
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
test: improve code coverage for streams/duplexify
aae131c 
PR-URL: nodejs/node#41862
Backport-PR-URL: nodejs/node#42788
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
juanarbol juanarbol approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

Back | FazBrowse Home | New Git URL