rapidsai / cudf

cuDF - GPU DataFrame Library
https://docs.rapids.ai/api/cudf/stable/
Apache License 2.0
8.33k stars 888 forks source link

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

Open vyasr opened 1 year ago

vyasr commented 1 year ago

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 https://github.com/rapidsai/cudf/pull/12888#discussion_r1132719780 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.

karthikeyann commented 1 year ago

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 commented 1 year ago

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).