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

["René Scharfe" <l.s.r@web.de>]

Am 23.07.24 um 08:36 schrieb Patrick Steinhardt:
> On Mon, Jul 22, 2024 at 12:36:57PM -0700, Junio C Hamano wrote:
>> Kyle Lippincott <spectral@google.com> writes:
>>>> +#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.
>
> Honestly I'm also not too sure how I feel about these new macros, and
> I'm somewhat in the same boat that it starts to feels "magicky".
>
> Taking a step back: what is it that is bugging folks about the current
> way of writing one function per test?

My goal is to be able to define a test without repeating its
description even partly, like test_expect_success allows for shell-based
tests.

> I quite liked the system
> that we had in libgit2: every function must conform to a specific name
> `test_foo_bar`. We then have a Python script that reads all test files,
> extracts all files that have the `test_` prefix, and writes those into
> an array. Optionally, the `test_foo` test suite can also have a setup
> and teardown function that gets called for every test, namely
> `test_foo_setup()` and `test_foo_teardown()`.
>
> Altogether, the output would look somewhat like this:
>
>
> ```test.c
> static test_foo_setup(void)
> {
>     ... setup global state ...
> }
>
> static test_foo_teardown(void)
> {
>     ... tear down global state ...
> }
>
> static test_foo_one(void)
> {
>     ... first test ...
> }
>
> static test_foo_two(void)
> {
>     ... second test ...
> }
> ```
>
> ```generated.c
> static const struct test_func _test_foo_functions[] = {
>     {
>         name: "foo::one",
>         test: test_foo_one,
>         setup: test_foo_setup,
>         teardown: test_foo_teardown,
>     },
>     {
>         name: "foo::two",
>         test: test_foo_two,
>         setup: test_foo_setup,
>         teardown: test_foo_teardown,
>     },
> };
>
> int main(int argc, const char *argv[])
> {
>     ... boilerplate to execute all functions ...
> }
> ```
>
> The setup and teardown functions are optional -- if not defined for a
> specific test unit, then we simply won't invoke them.

So the name of a test is generated automatically based on the function
name and there is no way to add a description beyond that.  This would
achieve my goal above, but prevent developers from using space and
punctuation in test descriptions.  Fair enough.

> There is of course some magic involved with how we generate the file.

It requires magic function names and generates code using a different
language, while for_test is a just single new control flow keyword,
like the dozen or so we already have.  So the magic employed by the
libgit2 system is both broader and deeper.

> But I think that would be quite manageable, and ultimately all that the
> developer would need to care about is writing a `test_foo_something()`
> function. Everything else would be handled by our infra.

With for_test all the developer has to do is write a test with a
description, no extra infrastructure beyond the existing unit test
framework needed.

> I'd be happy to implement something along these lines if folks think
> that this is a good direction to go into.

FWIW I don't see the appeal.  It uses more code and more dependencies
to almost allow what for_test permits.

René