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.

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

Improve C++ Test Coverage #920

Merged
merged 36 commits into from
Dec 1, 2021

Conversation

harrism
Copy link
Member

@harrism harrism commented Nov 17, 2021

Now that #905 added C++ code coverage support, this PR improves test coverage. Much of the improvement comes from adding a new adaptor_test.cpp which generically tests the common functions of all 7 adaptor types. Small test additions improve coverage of many other files as well. At least one typo bug was uncovered and fixed.

If you build with -DCODE_COVERAGE=ON -DRMM_LOGGING_LEVEL=TRACE, overall code coverage is now 99.5% and all but one file has 100% coverage. That one file, arena.hpp is undergoing work concurrent to this PR, and improvement to 100% requires additional testing that might be best undertaken by @rongou, so I will leave it.

image

@harrism harrism added tests Related to unit tests non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Nov 17, 2021
@harrism harrism self-assigned this Nov 17, 2021
@github-actions github-actions bot added CMake cpp Pertains to C++ code labels Nov 17, 2021
@harrism harrism marked this pull request as ready for review November 18, 2021 03:39
@harrism harrism requested review from a team as code owners November 18, 2021 03:39
@harrism
Copy link
Member Author

harrism commented Nov 23, 2021

Now up to 99% coverage without whitebox testing (and explicit instantiations uncovered more untested code, which I've fixed). I have a couple more things to try to fix a few more uncovered functions.

@harrism harrism added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Nov 24, 2021
@harrism
Copy link
Member Author

harrism commented Nov 24, 2021

Because a lot has been added since the reviewers approved, I'm rerequesting reviews.

include/rmm/logger.hpp Outdated Show resolved Hide resolved
include/rmm/mr/device/pool_memory_resource.hpp Outdated Show resolved Hide resolved
@harrism
Copy link
Member Author

harrism commented Dec 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d52c365 into rapidsai:branch-22.02 Dec 1, 2021
rapids-bot bot pushed a commit that referenced this pull request Dec 6, 2021
Fixes #928.

#920 introduced a new test for the `tracking_resource_adaptor::log_outstanding_allocations`, but that function only logs when `SPDLOG_ACTIVE_LEVEL <= SPDLOG_LEVEL_DEBUG`, so the test needs to be gated on that condition as well.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Rong Ou (https://github.com/rongou)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: #929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CMake cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change tests Related to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants