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

["René Scharfe" <l.s.r@web.de>]

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.

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

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

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é