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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Jul 2, 2024, 15:13 UTC
Message-ID: <51ebc0fd-ddae-4bdb-a5d6-c92ce8b1f3e6@gmail.com>
In-Reply-To: <8175f239-8d4e-49f7-ae0d-dba7df8c365d@web.de>

Hi René

On 29/06/2024 16:43, René Scharfe wrote:

Show 9 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

>   /*
>    * 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

Show 6 quoted lines

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