Re: [PATCH v3 02/10] trailer: add unit tests for trailer iterator

From: Linus Arver <linusa@google.com>
Date: Apr 26, 2024, 16:25 UTC
Message-ID: <owlywmok3xcy.fsf@fine.c.googlers.com>
In-Reply-To: <CAP8UFD0ZHpo7US6dx_WK6F_1JYsBdHunBaW86qM1CzOKaPC-aA@mail.gmail.com>

Hello Christian!

Christian Couder <christian.couder@gmail.com> writes:

Show 10 quoted lines

> On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Test the number of trailers found by the iterator (to be more precise,
>> the parsing mechanism which the iterator just walks over) when given
>> some some arbitrary log message.
>
> s/some some/some/

Fixed locally, thanks. Will send as part of a reroll pending further review comments.

Show 30 quoted lines

>> We test the iterator because it is a public interface function exposed
>> by the trailer API (we generally don't want to test internal
>> implementation details which are, unlike the API, subject to drastic
>> changes).
>>
>> Signed-off-by: Linus Arver <linusa@google.com>
>
>
>> +static void run_t_trailer_iterator(void)
>> +{
>> +       static struct test_cases {
>> +               const char *name;
>> +               const char *msg;
>> +               size_t num_expected_trailers;
>> +       } tc[] = {
>
> ...
>
>> +       };
>> +
>> +       for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) {
>> +               TEST(t_trailer_iterator(tc[i].msg,
>> +                                       tc[i].num_expected_trailers),
>> +                    "%s", tc[i].name);
>
> Nit: the members of struct test_cases are used in the (msg,
> num_expected_trailers, name) order, while they are declared in the
> (name, msg, num_expected_trailers) order. I think it would make it a
> bit easier to use in struct test_cases the same order in which they
> are used in the TEST() macro.

This bothered me as well, but ultimately I preferred to see the test names first in the actual test cases where each one is defined like

     {
        "name of test",
        ...
     },
     {
        "name of another test",
        ...
     }
     ...

instead of the other way around. FWIW this style comes from Golang where it is the standard practice there. I suppose in this instance we have test cases like

     {
             "without body text",
             "subject: foo bar\n"
             "\n"
             "Fixes: x\n"
             "Acked-by: x\n"
             "Reviewed-by: x\n",
             3
     },

and the separation between "name" vs "msg" could be a bit confusing on first glance, but I don't think that's a big deal. Plus our test_expect_success shell functions also expect the name as the first parameter, so it would be consistent with that style.

It's unfortunate that we cannot put __VA_ARGS__ as the "first parameter" to the TEST() macro, like

    TEST("%s", tc[i].name,
         t_trailer_iterator(tc[i].msg,
                            tc[i].num_expected_trailers),
        );

but I suppose that's a limitation of __VA_ARGS__. I also do wonder whether we even need the test case name to be __VA_ARGS__ at all though (we certainly don't *need* it here as the test case names are already unique) --- so it might be fine to have another macro that only takes the test name and a test function. Something like

    #define TC(name, t) ...

on top of the

    #define TEST(t, ...) ...

we already have, perhaps? IDK.