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

[phillip.wood123@gmail.com]

Hi René

On 05/07/2024 19:01, René Scharfe wrote:
> Am 05.07.24 um 11:42 schrieb phillip.wood123@gmail.com:
>> On 02/07/2024 21:55, René Scharfe wrote:
>>> Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com:
>>>
>>>> The changes in patch 6 to use TEST_RUN() mean that each test now has
>>>> more boilerplate to initialize and free the strbuf.
>>>
>>> This makes them more similar to strbuf usage in the wild.  Using
>>> the API idiomatically just makes more sense to me.
>>
>> I see what you mean. I think it only looks idiomatic if you're
>> already familiar with the api though as the test bodies call wrappers
>> rather than using the strbuf api directly. I think that reduces its
>> value as an example of idomatic usage for someone who is not familiar
>> with the strbuf api.
> 
> In early versions I used the original names by adding these just before
> main():
> 
>     #define strbuf_addch(x, y)  t_addch(x, y)
>     #define strbuf_addstr(x, y) t_addstr(x, y)
>     #define strbuf_release(x)   t_release(x)
> 
> This allowed normal looking code to be used in tests, with checks
> injected behind the scenes.  Rejected it for v1 because it offers no
> structural improvements, just optics.  It does allow to forget the
> checked versions when writing tests, though, so perhaps it's still
> worth doing.

It also obscures what the test is doing though. It's nice if unit tests 
show how to use an API but I don't think we should let that be our 
overriding concern.

>>> We could also let xstrvfmt() call vsnprintf(3) directly.  The code
>>> duplication would be a bit grating, but perhaps there's some good
>>> base function hidden in there somewhere.
>>
>> Oh, interesting - maybe something like
>>
>> char* xstrvfmt(const char *fmt, ...)
>> {
>>      va_list ap, aq;
>>
>>      va_start(ap, fmt);
>>      va_copy(aq, ap);
>>      len = vnsprintf(NULL, 0, fmt, ap);
>>      if (len < 0)
>>          BUG(...)
>>      buf = xmalloc(len + 1);
>>      if (vnsprintf(buf, len + 1, fmt, aq) != len)
>>          BUG(...)
>>      va_end(aq);
>>      va_end(ap);
>>
>>      return buf;
>> }
> 
> Yes.  Though the current version allocates 65 bytes in the first try
> and only needs to call vnsprintf(3) once if the output fits in.  No
> longer doing that might affect the performance of some callers in a
> noticeable way.

Good point

>>>> I'm not very enthusiastic about requiring the test author to wrap
>>>> TEST_RUN() in an if() statement rather than just doing that for them.
>>>> It makes it explicit but from the test author's point of view it just
>>>> feels like pointless boilerplate.
>>>
>>> Hmm.  We can add more magic, but I suspect that it might confuse
>>> developers and editors.
>>
>> To me its confusing to have to wrap TEST_RUN() in an if() statement
>> until one realizes that the test might be skipped. If we document
>> that the test body should be enclosed in braces I don't think it
>> should confuse developers or editors and will keep the tests a bit
>> cleaner.
> 
> You don't need braces in either case.  I.e. something like
> 
> 	if (TEST_RUN("foo"))
> 		foo();
> 
> works fine.  And
> 
> 	#define IF_TEST_RUN(...) if (TEST_RUN(__VA_ARGS__))
> 	IF_TEST_RUN("foo")
> 		foo();
> 
> works fine as well.

Indeed. The reason I suggested requiring braces was to help editors 
indent the code correctly and because it gives a nice visual cue to see 
what is in each test.

Best Wishes

Phillip

> The confusion that I'm afraid of is that we'd effectively invent a new
> control flow keyword here, which is unusual.  There are precedents,
> though: foreach macros like for_each_string_list_item.  We tell
> clang-format about them using its option ForEachMacros.  I see it also
> has an option IfMacros since version 13 (unused by us so far).  Hmm.
> 
> René