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

[phillip.wood123@gmail.com]

Hi René

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. The changes in patch 6 to use 
TEST_RUN() mean that each test now has more boilerplate to initialize 
and free the strbuf. 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.

On the other hand having all the tests defined in main() using 
TEST_RUN() means we can just write the test body without having to 
define a separate function and then call it with TEST()

> No public method is provided for ending a test explicitly, yet; let's
> see if we'll ever need one.

This means that we do not error out if there are accidentally nested 
tests. That probably does not matter too much.


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

> +/*
> + * Start a test, returns 1 if the test was actually started or 0 if it
> + * was skipped.  The test ends when the next test starts or test_done()
> + * is called.
> + */
> +#define TEST_RUN(...) test__run(TEST_LOCATION(), __VA_ARGS__)

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.

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.

>   /*
>    * test_done() must be called at the end of main(). It will print the
>    * plan if plan() was not called at the beginning of the test program
> @@ -156,6 +163,7 @@ extern union test__tmp test__tmp[2];
>   int test__run_begin(void);
>   __attribute__((format (printf, 3, 4)))
>   int test__run_end(int, const char *, const char *, ...);

We should add

     __attribute__((format (printf, 2, 3), warn_unused_result))

here to catch any errors in the format string / arguments and to warn if 
TEST_RUN() isn't wrapped in an if() statement.

Best Wishes

Phillip

> +int test__run(const char *location, const char *format, ...);
>   void test__todo_begin(void);
>   int test__todo_end(const char *, const char *, int);
> 
> --
> 2.45.2