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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 31, 2025, 11:43 UTC
Message-ID: <Z5y3f1B450LZOp3v@pks.im>
In-Reply-To: <20250130091334.39922-4-kuforiji98@gmail.com>

On Thu, Jan 30, 2025 at 10:13:33AM +0100, Seyi Kuforiji wrote:

> Adapt strbuf test script to clar framework by using clar assertions
> where necessary. Test functions are created as standalone to test
> different test cases.

Same comment here regarding the second sentence.

Show 8 quoted lines

> diff --git a/t/unit-tests/u-strbuf.c b/t/unit-tests/u-strbuf.c
> new file mode 100644
> index 0000000000..fec3c768d2
> --- /dev/null
> +++ b/t/unit-tests/u-strbuf.c
> @@ -0,0 +1,121 @@
> +#include "unit-test.h"
> +#include "strbuf.h"

It's somewhat sad that Git doesn't not think of this as a rename, as it would've made the diff easier to read. You might want to try using `git format-patch --find-renames=20' to ask Git to still consider this a rename.

[snip]

Show 14 quoted lines

> +static void assert_sane_strbuf(struct strbuf *buf)
> +{
> +	/* Initialized strbufs should always have a non-NULL buffer */
> +	cl_assert(buf->buf != NULL);
> +	/* Buffers should always be NUL-terminated */
> +	cl_assert(buf->buf[buf->len] == '\0');
> +	/*
> +	 * Freshly-initialized strbufs may not have a dynamically allocated
> +	 * buffer
> +	 */
> +	if (buf->len == 0 && buf->alloc == 0)
> +		return;
> +	/* alloc must be at least one byte larger than len */
> +	cl_assert(buf->len < buf->alloc);

I think this condition can be simplified a bit:

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

Show 6 quoted lines

> +void test_strbuf__add_empty_char(void)
> +{
> +	setup(t_addch, "");
> +}
> +
> +void test_strbuf__add_multi_char(void)

Nit: I think "add_multi_char" is slightly misleading. What this test
exercises is that we can append to a non-empty buffer. So maybe
"append_char" would be a better fit?

Show 10 quoted lines

> +{
> +	setup_populated(t_addch, "initial value", "a");
> +}
> +
> +void test_strbuf__add_single_str(void)
> +{
> +	setup(t_addstr, "hello there");
> +}
> +
> +void test_strbuf__add_multi_str(void)

Similarly, we could name this "append_str".

> +{
> +	setup_populated(t_addstr, "initial value", "hello there");
> +}

Patrick