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

From: Kyle Lippincott <spectral@google.com>
Date: Jul 22, 2024, 22:41 UTC
Message-ID: <CAO_smViu2AtdU3GceEJkGXK-pXZr+QyvrWk5sYRVLypyFp1PeA@mail.gmail.com>
In-Reply-To: <xmqq1q3lb4me.fsf@gitster.g>

On Mon, Jul 22, 2024 at 12:37 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

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

Perhaps expression_test or something, then? It's testing an expression (I think blocks are a type of expression? I never remember which is 'larger': expressions or statements, and that might only be a C++ thing, C might not have the same terminology).

I was going to suggest a lint check that checks to ensure that we don't have a semicolon immediately after the description, or a lint check that enforced that even single-statement tests are wrapped in {} (inverting the style guide requirements), but realistically neither are actually needed: `test__run_begin` already asserts that a test isn't currently running. `check_int` and friends already assert that a test _is_ running. So this is already defended against:

for_test ("test description");
    check_int(1, ==, 1);   /* `assert`s: not in the middle of a test */

What we don't have is defense against a completely empty test, or a test without any actual conditions:

for_test ("test one");
for_test ("test two") {
    printf("this test doesn't actually _test_ anything\n");
}

Adding that is doable, and improves the first case: the `for_test ("test description");` line fails because the test didn't do anything, not the `check_int` line.

Show 50 quoted lines

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