Re: [PATCH v2 2/6] unit-tests: add for_test

[Junio C Hamano <gitster@pobox.com>]

Kyle Lippincott <spectral@google.com> writes:

> I can see based on this description where the name came from, but
> without this context, it's not clear when reading a test what it
> actually does. The name comes from an implementation detail, and is
> not describing what it _does_, just _how_ it does it.
>
> Maybe a name like `small_test` or `quick_test` or something like that
> would better express the intended usage?

Names that explicitly have C keyword for control structures, e.g.
"if_somecondition_test()", "while_foo_test()" or "for_test()" is
vastly preferrable than these, in order to make it clear that we are
introducing a macro that defines control structure.

>> +       for_test ("for_test passing test")
>> +               check_int(1, ==, 1);
>
> I'm concerned that users will write this like:
> +       for_test ("for_test passing test");
> +               check_int(1, ==, 1);

And that is exactly why we want the macro name to include C keyword 
for control structures.

> And the issue won't be caught.

You are right.  Making an empty body somehow catchable by the
compiler would be a vast improvement.

>> +#define for_test(...)                                                  \
>> +       for (int for_test_running_ = test__run_begin() ?                \
>> +               (test__run_end(0, TEST_LOCATION(), __VA_ARGS__), 0) : 1;\
>> +            for_test_running_;                                         \
>> +            test__run_end(1, TEST_LOCATION(), __VA_ARGS__),            \
>> +               for_test_running_ = 0)
>
> IMHO: this is borderline "too much magic" for my tastes. I think
> having multiple test functions is generally easier to understand, and
> the overhead isn't really relevant. It's not _as_ compact in the
> source file, and requires that we have both the TEST statement and the
> function (and forgetting the TEST statement means that we won't invoke
> the function). If that is the main issue we're facing here, I wonder
> if there's some other way of resolving that (such as unused function
> detection via some compiler flags; even if it's not detected on all
> platforms, getting at least one of the CI platforms should be
> sufficient to prevent the issue [but we should target as many as
> possible, so it's caught earlier than "I tried to send it to the
> list"])

Interesting.

> If others agree that this is a good simplification for the people
> reading the test code (and hopefully for the test author), I'm fine
> with this going in (with a different name). I'm not trying to veto the
> concept.

OK.  But what you suggested in the previous paragraph has merit.
Are there other things that could be improved in the existing unit
test helpers, that would help those who do not use this new for_test()
thing?  Let's see how the patches to improve them would look like.

Thanks.