From: phillip.wood123@gmail.com
Date: Fri, 05 Jul 2024 09:42:29 GMT
Subject: Re: [PATCH 2/6] unit-tests: add TEST_RUN
Message-ID: <8802b3ab-9986-47bd-be80-51ac599b2892@gmail.com>
In-Reply-To: <97390954-49bc-48c4-bab1-95be10717aca@web.de>

Hi René

On 02/07/2024 21:55, René Scharfe wrote:
> Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com:
>> On 29/06/2024 16:43, René Scharfe wrote:
>>> The macro TEST only allows defining a test that consists of a
>>> single expression.  Add the new macro, TEST_RUN, which provides a
>>> way to define unit tests that are made up of one or more
>>> statements.  A test started with it implicitly ends when the next
>>> test is started or test_done() is called.
>>>
>>> TEST_RUN allows defining self-contained tests en bloc, a bit like
>>> test_expect_success does for regular tests.  Unlike TEST it does
>>> not require defining wrapper functions for test statements.
>>
>> There are pros and cons to not requiring one function per test. It
>> can be a pain to have to write separate functions but it keeps each
>> test self contained which hopefully makes it harder to have
>> accidental dependencies between tests. Having separate functions for
>> each test makes it easy to initialize and free resources for every
>> test by writing a setup() function that initializes the resources,
>> calls the test function and then frees the resources.
> 
> Right.  We should use TEST and TEST_RUN when appropriate.
> 
>> 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.

>  Not hiding
> initialization and release makes the tests visibly independent.
> This is not enforced by TEST_RUN, but made possible by it.
> 
>> Having each test in its own function also makes main() shorter and
>> which means can quickly get an overview of all the test cases from
>> it.
> 
> That's true, now you need to grep for TEST_RUN to get such an
> overview.
> 
> On the other hand I find the start of the description in TEST
> invocations somewhat hard to locate, as they are not vertically
> aligned due to the preceding variable-length function name.  Just
> saying..

Yes I really wanted the first argument of TEST to be the description but 
that isn't easy to do while supporting printf style format strings.

>>> +int test__run(const char *location, const char *format, ...)
>>> +{
>>> +    va_list ap;
>>> +    char *desc;
>>> +
>>> +    test__run_maybe_end();
>>> +
>>> +    va_start(ap, format);
>>> +    desc = xstrvfmt(format, ap);
>>
>> This uses an strbuf under the hood. So far we've avoided doing that
>> as we want to be able to test the strbuf implementation with this
>> test framework. We don't need to support arbitrary length strings
>> here so we could use a fixed array and xsnprinf() instead.
> 
> Fair point.  xsnprinf() might be a bit too strict, as it doesn't
> handle short buffers gracefully.  Perhaps that's OK; a developer
> getting hit by that could simply increase the buffer size.

I think so.

> 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;
}

>> Looking ahead the plan seems to be to change most instances of TEST()
>> to TEST_RUN(). If we are going to go that way perhaps we should steal
>> TEST() for this macro and rename the existing TEST() macro.
> 
> Not my plan, at least -- I'm content with just having the *ability*
> to keep all parts of a test together.

That sounds sensible to me

> +	if (TEST_RUN("static initialization works")) {
> +		struct strbuf buf = STRBUF_INIT;
> +		if (!check_uint(buf.len, ==, 0) ||
> +		    !check_uint(buf.alloc, ==, 0) ||
> +		    !check_char(buf.buf[0], ==, '\0'))
> +			test_skip_all("STRBUF_INIT is broken");
> +	}

that's a nice use of test_skip_all()

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

Best Wishes

Phillip