Hello Christian!
Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>>
>> From: Linus Arver <linusa@xxxxxxxxxx>
>>
>> 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.
>> 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@xxxxxxxxxx>
>
>
>> +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.
