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

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

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

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

Right.

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

I am not sure if I agree.  In the array of struct, being able to
identify each array item with its .name component makes quite a lot
of sense, especially when the .name member is not really part of the
data used in tests but is used as an identifier for the tuple made
of other members (i.e., <msg, num_expected_trailers> in this case).

The TEST() macro is unable to take "name" as an early parameter than
others due to how it wants to create the identifying string (i.e.,
doing an equivalent of strfmt() on tc[i].name), but reordering the
struct members to match the peculiar order the members are used
smells like a tail wagging a dog.

>
>> +       }
>> +}
>> +
>> +int cmd_main(int argc, const char **argv)
>> +{
>> +       run_t_trailer_iterator();
>> +       return test_done();
>> +}
>
> LGTM otherwise.

Thanks.