Re: [PATCH 5/6] t-strvec: use TEST_RUN

From: René Scharfe <l.s.r@web.de>
Date: Jul 2, 2024, 20:55 UTC
Message-ID: <fc83c8da-4808-4e52-bfda-6b2d0c5cb553@web.de>
In-Reply-To: <c122c0a6-9f38-489d-8286-b62bd4deefcc@gmail.com>

Am 02.07.24 um 17:14 schrieb phillip.wood123@gmail.com:

Show 13 quoted lines

> Hi René
>
> On 29/06/2024 16:46, René Scharfe wrote:
>> The macro TEST takes a single expression.  If a test requires
>> multiple statements then they need to be placed in a function
>> that's called in the TEST expression.
>>
>> Remove the overhead of defining and calling single-use functions
>> by using TEST_RUN instead.
>
> I'm not sure what the overhead is that you keep referring to - the
> compiler will inline functions that are only called once so I presume
> you mean overhead for test authors or contributors reading the code?

Yes, I mean the additional effort for developers to come up with function names and write single-call functions, and for readers to mentally connect test description and function. Of course you still can do that with TEST_RUN, but unlike TEST it doesn't force you.

I don't care much about changed duration of compilation or execution here -- it shouldn't be much either way.

Show 16 quoted lines

> This is not related to you changes but while looking at this patch I
> noticed that the existing code in check_strvec_loc() contains
>
>
>         if (!check_uint(vec->nr, >, nr) ||
>             !check_uint(vec->alloc, >, nr) ||
>             !check_str(vec->v[nr], str)) {
>             struct strbuf msg = STRBUF_INIT;
>             strbuf_addf(&msg, "strvec index %"PRIuMAX, (uintmax_t) nr);
>             test_assert(loc, msg.buf, 0);
>             strbuf_release(&msg);
>             va_end(ap);
>             return;
>
> Which looks like it should be using test_msg() instead of writing a
> message to an strbuf and calling test_assert().

Erm, yes, good find. You know something isn't right if you see an assert(0) or similar..

> The conversion itself looks correct. It is a shame that each test has
> to have boilerplate to initalize and free the strvec but that comes
> from the existing implementation.

I see that as a benefit: You can see what each test does at a glance; no tricks, no dependencies. Just exercise steps and checks.

René