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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4437 bytes --]

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? The boilerplate cannot really be
it, as tests should be self-contained anyway and thus we should have the
same boilerplate regardless of whether we use separate functions or the
new macros. I also doubt that it's the function declaration itself, as
that isn't all that involved. So I guess what people are annoyed with is
that every function needs to be defined, but then also wired up via
`TEST_RUN()`, right? And that I totally get, as it is quite annoying.

So... can we solve this in a different way? 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.

There is of course some magic involved with how we generate the file.
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.

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]