[libc-commits] [PATCH] D100631: [libc] Extends the testing framework to support typed test

Fri Apr 16 10:08:12 PDT 2021

sivachandra added a comment.

In D100631#2695208 <https://reviews.llvm.org/D100631#2695208>, @gchatelet wrote:

> In D100631#2695126 <https://reviews.llvm.org/D100631#2695126>, @sivachandra wrote:
>
>> While I am not opposed to this, I want to learn the usefulness of this feature. IIUC, this limits to a single `ParamType` - as in a uni-dimensional type variance. We have examples in math functions where there is two-dimensional type variance. I am not sure the feature added in this patch can help with it. For such cases, have used template test classes and helper macros to list the various combination of tests: https://github.com/llvm/llvm-project/blob/main/libc/test/src/math/RoundToIntegerTest.h.
>
> Yes I've spotted these tests but I found that it scaled poorly as you define more tests since you have to define the tests into a macro which makes for poor editing.
> You ended up moving the body of the test to an external function to help with this but I think it's not ideal (especially for failure location).

There is no problem with failure location - you will get the correct file and line number. With math tests we do not have a problem with scaling also - we want tests for each type combination to be in different files as they correspond to different functions.
Also, I am not sure what you mean by "define tests in a macro".  The tests are member functions of the template class. The listing part is done with a macro, which is minor.

> In the case of multi-dimensional types you can pack them into a tuple and unpack them in the test fixture. I'll try to implement `RoundToIntegerTest` with `TYPED_TEST` and let you know how it looks like. My particular use case is for 19 types and 4 tests <https://reviews.llvm.org/differential/changeset/?ref=2515932> (but more to come).

If you find it useful, I am totally fine with it. Lets wait to hear from phosek about adding this to zxtest.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100631/new/

https://reviews.llvm.org/D100631