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

[FEA] Investigate removing C++ tests of detail APIs #12929

Open
vyasr opened this issue Mar 13, 2023 · 2 comments
Open

[FEA] Investigate removing C++ tests of detail APIs #12929

vyasr opened this issue Mar 13, 2023 · 2 comments
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.

Comments

@vyasr
Copy link
Contributor

vyasr commented Mar 13, 2023

Is your feature request related to a problem? Please describe.
Some C++ test files are written to test detail APIs rather than public APIs. One particular example that was brought up in #12888 (comment) is detail_gather_tests.cu. Ideally we should not be testing detail APIs directly, only public APIs. In practice, we should minimize cases where we must test detail APIs directly.

Describe the solution you'd like
We should evaluate removing tests of detail APIs. In cases where the associated public APIs are not sufficiently tested, the detail tests should be converted to test the public APIs. In other cases where the detail tests are purely redundant they should be removed. If detail APIs are being called as part of a sequence of cudf calls in a more complex test of public APIs, those calls should be rewritten to use public APIs. The remainder should be cases where detail APIs lack an exact public analog and testing the underlying APIs is valuable. We will need to assess those tests carefully.

@vyasr vyasr added feature request New feature or request Needs Triage Need team to review and classify labels Mar 13, 2023
@karthikeyann
Copy link
Contributor

karthikeyann commented Mar 13, 2023

Some parts of large code/algorithm, are available as detail APIs, but not as public APIs, and they could use some fundamental unit tests. These unit tests helps to build the top level public APIs without any errors, and also helps figuring out the errors easily when some tests fail.
Detail APIs are exposed in include/cudf/ headers, so, it is good to test them too, unless it's exactly identical to public APIs (except the stream parameter).

@vyasr
Copy link
Contributor Author

vyasr commented Mar 15, 2023

I agree, my original wording was probably a bit too strong. I don't necessarily think that we should remove all tests of detail APIs, but we should be much more judicious about using them if tests of public APIs would be sufficient. Where possible, we should convert them to testing the corresponding public APIs (if one exists).

@GregoryKimball GregoryKimball added code quality libcudf Affects libcudf (C++/CUDA) code. 0 - Backlog In queue waiting for assignment and removed Needs Triage Need team to review and classify labels Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

No branches or pull requests

3 participants