From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 18 May 2023 13:32:51 GMT
Subject: Re: [PATCH RFC v2 4/4] unit test: add basic example and build rules
Message-ID: <e26789b2-21fd-668d-ee00-2640d5b8d5c2@gmail.com>
In-Reply-To: <20230517-unit-tests-v2-v2-4-21b5b60f4b32@google.com>

On 18/05/2023 00:56, steadmon@google.com wrote:
> Integrate a simple strbuf unit test with Git's Makefiles.
> 
> You can build and run the unit tests with `make unit-tests` (or just
> build them with `make build-unit-tests`). By default we use the basic
> test runner from the C-TAP project, but users who prefer prove as a test
> runner can set `DEFAULT_UNIT_TEST_TARGET=prove-unit-tests` instead.
> 
> We modify the `#include`s in the C TAP libraries so that we can build
> them without having to include the t/ directory in our include search
> path.

Thanks for adding some example tests, it is really helpful to see how 
the library will be used.

I tried building the units test with SANITIZE=address set and I get lots 
of link errors complaining about undefined references to __asan_*

> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> Change-Id: Ie61eafd2bd8f8dc5b30449af1e436889f91da3b7

> diff --git a/t/strbuf-test.c b/t/strbuf-test.c
> new file mode 100644
> index 0000000000..8f8d4e11db
> --- /dev/null
> +++ b/t/strbuf-test.c
> @@ -0,0 +1,54 @@
> +#include "tap/basic.h"
> +
> +#include "../git-compat-util.h"
> +#include "../strbuf.h"
> +
> +int strbuf_init_test()
> +{
> +	struct strbuf *buf = malloc(sizeof(void*));

Is there a reason to use dynamic allocation here. Also I think you need 
sizeof(*buf) to allocate the correct size.

> +	strbuf_init(buf, 0);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 0)
> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}

This test nicely illustrates why I'd prefer a different approach. The 
test author has to maintain the pass/fail state and there are no 
diagnostics if it fails to tell you which check failed. To be clear I 
view the lack of diagnostics as the fault of the test framework, not the 
test author. I'd prefer something like

	void strbuf_init_test(void)
	{
		struct strbuf buf;

		strbuf_init(&buf, 0);
		check_char(buf.buf[0] == '\0');
		check_uint(buf.alloc, ==, 0);
		check_uint(buf.len, ==, 0);
	}

which would be run as

	TEST(strbuf_init_test(), "strbuf_init initializes properly");

in main() and provide diagnostics like

     # check "buf.alloc == 0" failed at my-test.c:102
     #    left: 2
     #   right: 0

when a check fails.

> +int strbuf_init_test2() {
> +	struct strbuf *buf = malloc(sizeof(void*));
> +	strbuf_init(buf, 100);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 101)

Strictly speaking I think the API guarantees that at least 100 bytes 
will be allocated, not the exact amount as does alloc_grow() below.

> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}
> +
> +
> +int strbuf_grow_test() {
> +	struct strbuf *buf = malloc(sizeof(void*));
> +	strbuf_grow(buf, 100);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 101)
> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}

Best Wishes

Phillip