Re: [PATCH v2 3/4] t/unit-tests: convert strbuf test to use clar test framework

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Seyi

On 31/01/2025 22:14, Seyi Kuforiji wrote:
> Adapt strbuf test script to clar framework by using clar assertions
> where necessary.

This patch looks correct but it looses the nice messages we get from 
check_char() and check_uint(a, </<=/>/>=, b). I think it would be worth 
adding equivalent assertions to clar to avoid that.

>   /* wrapper that supplies tests with an empty, initialized strbuf */
> @@ -9,8 +9,8 @@ static void setup(void (*f)(struct strbuf*, const void*),
>   
>   	f(&buf, data);
>   	strbuf_release(&buf);
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, ==, 0);
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert_equal_i(buf.alloc, 0);

It is a shame that we do not have an assertion for checking two unsigned 
integers are equal. In this case it probably does not matter but large 
unsigned values will be printed as negative numbers which could be 
rather confusing for someone trying to debug a test failure.

> -static int assert_sane_strbuf(struct strbuf *buf)
> +static void assert_sane_strbuf(struct strbuf *buf)
>   {
>   	/* Initialized strbufs should always have a non-NULL buffer */
> -	if (!check(!!buf->buf))
> -		return 0;
> +	cl_assert(buf->buf != NULL);
>   	/* Buffers should always be NUL-terminated */
> -	if (!check_char(buf->buf[buf->len], ==, '\0'))
> -		return 0;
> +	cl_assert(buf->buf[buf->len] == '\0');

It is unfortunate that this looses the helpful diagnostic message that 
shows the values of the two chars being compared that is printed by 
check_char() when the check fails. I think it would be worth porting 
check_char() arcoss to clar. When we decided to move to the new test 
framework we planned to add new assertions to clar to match our existing 
framework.

>   	/*
> -	 * Freshly-initialized strbufs may not have a dynamically allocated
> -	 * buffer
> -	 */
> -	if (buf->len == 0 && buf->alloc == 0)
> -		return 1;
> -	/* alloc must be at least one byte larger than len */
> -	return check_uint(buf->len, <, buf->alloc);
> +         * In case the buffer contains anything, `alloc` must alloc must
> +         * be at least one byte larger than `len`.
> +         */
> +	if (buf->len)
> +            cl_assert(buf->len < buf->alloc);

This is another case where we loose a helpful diagnostic message because 
clar lacks cl_assert_lt_u(a, b) to check that unsigned integer a is less 
than unsigned integer b. I think it would be worth adding the assertions 
that are missing show that we have the equivalent of

	check_int(a, <, b)
	check_int(a, <=, b)
	check_int(a, >, b)
	check_int(a, >=, b)

and their unsigned companions so we do not regress the diagnostic output 
when a test fails.

Looking at the comment that is deleted above I wonder if we should add

	else
		cl_assert_equal_i(buf->alloc, 0);

To check that alloc is zero when len is zero.

Best Wishes

Phillip

>   }
>   
> -static void t_static_init(void)
> +void test_strbuf__static_init(void)
>   {
>   	struct strbuf buf = STRBUF_INIT;
>   
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, ==, 0);
> -	check_char(buf.buf[0], ==, '\0');
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert_equal_i(buf.alloc, 0);
> +	cl_assert(buf.buf[0] == '\0');
>   }
>   
> -static void t_dynamic_init(void)
> +void test_strbuf__dynamic_init(void)
>   {
>   	struct strbuf buf;
>   
>   	strbuf_init(&buf, 1024);
> -	check(assert_sane_strbuf(&buf));
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, >=, 1024);
> -	check_char(buf.buf[0], ==, '\0');
> +	assert_sane_strbuf(&buf);
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert(buf.alloc >= 1024);
> +	cl_assert(buf.buf[0] == '\0');
>   	strbuf_release(&buf);
>   }
>   
> @@ -73,16 +69,12 @@ static void t_addch(struct strbuf *buf, const void *data)
>   	size_t orig_alloc = buf->alloc;
>   	size_t orig_len = buf->len;
>   
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> +	assert_sane_strbuf(buf);
>   	strbuf_addch(buf, ch);
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> -	if (!(check_uint(buf->len, ==, orig_len + 1) &&
> -	      check_uint(buf->alloc, >=, orig_alloc)))
> -		return; /* avoid de-referencing buf->buf */
> -	check_char(buf->buf[buf->len - 1], ==, ch);
> -	check_char(buf->buf[buf->len], ==, '\0');
> +	assert_sane_strbuf(buf);
> +	cl_assert_equal_i(buf->len, orig_len + 1);
> +	cl_assert(buf->alloc >= orig_alloc);
> +	cl_assert(buf->buf[buf->len] == '\0');
>   }
>   
>   static void t_addstr(struct strbuf *buf, const void *data)
> @@ -92,31 +84,36 @@ static void t_addstr(struct strbuf *buf, const void *data)
>   	size_t orig_alloc = buf->alloc;
>   	size_t orig_len = buf->len;
>   
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> +	assert_sane_strbuf(buf);
>   	strbuf_addstr(buf, text);
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> -	if (!(check_uint(buf->len, ==, orig_len + len) &&
> -	      check_uint(buf->alloc, >=, orig_alloc) &&
> -	      check_uint(buf->alloc, >, orig_len + len) &&
> -	      check_char(buf->buf[orig_len + len], ==, '\0')))
> -	    return;
> -	check_str(buf->buf + orig_len, text);
> +	assert_sane_strbuf(buf);
> +	cl_assert_equal_i(buf->len, orig_len + len);
> +	cl_assert(buf->alloc >= orig_alloc);
> +	cl_assert(buf->buf[buf->len] == '\0');
> +	cl_assert_equal_s(buf->buf + orig_len, text);
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_strbuf__add_single_char(void)
>   {
> -	if (!TEST(t_static_init(), "static initialization works"))
> -		test_skip_all("STRBUF_INIT is broken");
> -	TEST(t_dynamic_init(), "dynamic initialization works");
> -	TEST(setup(t_addch, "a"), "strbuf_addch adds char");
> -	TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
> -	TEST(setup_populated(t_addch, "initial value", "a"),
> -	     "strbuf_addch appends to initial value");
> -	TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
> -	TEST(setup_populated(t_addstr, "initial value", "hello there"),
> -	     "strbuf_addstr appends string to initial value");
> -
> -	return test_done();
> +	setup(t_addch, "a");
> +}
> +
> +void test_strbuf__add_empty_char(void)
> +{
> +	setup(t_addch, "");
> +}
> +
> +void test_strbuf__add_append_char(void)
> +{
> +	setup_populated(t_addch, "initial value", "a");
> +}
> +
> +void test_strbuf__add_single_str(void)
> +{
> +	setup(t_addstr, "hello there");
> +}
> +
> +void test_strbuf__add_append_str(void)
> +{
> +	setup_populated(t_addstr, "initial value", "hello there");
>   }