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: for hasItems method in FreeList #27588

Closed
wants to merge 1 commit into from
Closed

test: for hasItems method in FreeList #27588

wants to merge 1 commit into from

Conversation

rpgeeganage
Copy link
Contributor

This PR is for missing tests for hasItems() method in /internal/freelist.js.

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 May 6, 2019
Trott
Trott approved these changes May 7, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM but I'd still prefer we use the public APIs rather than add to tests that dig into internal flag-protected APIs like this. For example, this could be exercised with require('_http_common').parsers.hasItems().

@rpgeeganage
Copy link
Contributor Author

@Trott ,
If you wish, I can refactor the entire test file test/parallel/test-freelist.js to use your suggestion. Or do you wish to change only the new tests?

@targos
Copy link
Member

targos commented May 7, 2019

I think it's fine to test an internal utility in this way.

@BridgeAR
Copy link
Member

BridgeAR commented May 7, 2019

I do agree with @Trott that it would be better to test these things with public APIs. Otherwise it's difficult to detect actual dead code by looking at coverage reports. It gives the false impression that it's used while the code branch might not ever be reached with our actual code. Somewhat similar about the coverage it results in: testing internal functionalities directly instead of using public APIs might take different code paths and thus result in errors even though it seems fully tested.

In this specific case it seems fine but in general I would like to reduce testing internals directly.

@rpgeeganage
Copy link
Contributor Author

@BridgeAR ,
Thank you very much for the really important point.
I'll keep this in my mind, next time I check the coverage report.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented May 10, 2019

Landed in ac56dc9.

@Trott Trott closed this May 10, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request May 10, 2019
@rpgeeganage @Trott
test: add tests for hasItems method in FreeList
ac56dc9 
PR-URL: nodejs#27588
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request May 11, 2019
@rpgeeganage @targos
test: add tests for hasItems method in FreeList
3801859 
PR-URL: #27588
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
Trott added a commit to Trott/io.js that referenced this pull request Nov 30, 2019
@Trott
test: replace internal API test with public API test
925f55c 
The internal freelist `hasItems()` test use the internal API directly.
This commit changes them to use the API as it is exposed publicly. It
tests the same code paths, but does it without needing
`--expose-internals`.

Refs: nodejs#27588 (review)
@Trott Trott mentioned this pull request Nov 30, 2019
Closed
3 tasks
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

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott 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.
8 participants
@rpgeeganage @targos @BridgeAR @nodejs-github-bot @Trott @addaleax @cjihrig @trivikr