Re: [PATCH v2 02/10] t/unit-tests: convert reftable basics test to use clar test framework

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 29, 2025 at 06:52:54PM +0100, Seyi Kuforiji wrote:
> Adapt reftable basics test file to clar by using clar assertions
> where necessary.Break up test edge case to improve modularity and
> clarity.
> 
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  Makefile                         |   2 +-
>  t/meson.build                    |   2 +-
>  t/unit-tests/t-reftable-basics.c | 219 -------------------------------
>  t/unit-tests/u-reftable-basics.c | 195 +++++++++++++++++++++++++++
>  4 files changed, 197 insertions(+), 221 deletions(-)
>  delete mode 100644 t/unit-tests/t-reftable-basics.c
>  create mode 100644 t/unit-tests/u-reftable-basics.c
> 
> diff --git a/Makefile b/Makefile
> index 13f9062a05..7b12bb078c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1362,6 +1362,7 @@ CLAR_TEST_SUITES += u-oid-array
>  CLAR_TEST_SUITES += u-oidmap
>  CLAR_TEST_SUITES += u-oidtree
>  CLAR_TEST_SUITES += u-prio-queue
> +CLAR_TEST_SUITES += u-reftable-basics
>  CLAR_TEST_SUITES += u-reftable-tree
>  CLAR_TEST_SUITES += u-strbuf
>  CLAR_TEST_SUITES += u-strcmp-offset
> @@ -1374,7 +1375,6 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
>  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
>  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
>  
> -UNIT_TEST_PROGRAMS += t-reftable-basics
>  UNIT_TEST_PROGRAMS += t-reftable-block
>  UNIT_TEST_PROGRAMS += t-reftable-merged
>  UNIT_TEST_PROGRAMS += t-reftable-pq
> diff --git a/t/meson.build b/t/meson.build
> index bfb744e886..8a42b595d9 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -8,6 +8,7 @@ clar_test_suites = [
>    'unit-tests/u-oidmap.c',
>    'unit-tests/u-oidtree.c',
>    'unit-tests/u-prio-queue.c',
> +  'unit-tests/u-reftable-basics.c',
>    'unit-tests/u-reftable-tree.c',
>    'unit-tests/u-strbuf.c',
>    'unit-tests/u-strcmp-offset.c',
> @@ -54,7 +55,6 @@ clar_unit_tests = executable('unit-tests',
>  test('unit-tests', clar_unit_tests)
>  
>  unit_test_programs = [
> -  'unit-tests/t-reftable-basics.c',
>    'unit-tests/t-reftable-block.c',
>    'unit-tests/t-reftable-merged.c',
>    'unit-tests/t-reftable-pq.c',
> diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c
> deleted file mode 100644
> index c9e751e49e..0000000000
> --- a/t/unit-tests/t-reftable-basics.c
> +++ /dev/null
> @@ -1,219 +0,0 @@
> -/*
> -Copyright 2020 Google LLC
> -
> -Use of this source code is governed by a BSD-style
> -license that can be found in the LICENSE file or at
> -https://developers.google.com/open-source/licenses/bsd
> -*/
> -
> -#include "test-lib.h"
> -#include "reftable/basics.h"
> -
> -struct integer_needle_lesseq_args {
> -	int needle;
> -	int *haystack;
> -};
> -
> -static int integer_needle_lesseq(size_t i, void *_args)
> -{
> -	struct integer_needle_lesseq_args *args = _args;
> -	return args->needle <= args->haystack[i];
> -}
> -
> -static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
> -{
> -	return NULL;
> -}
> -
> -int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
> -{
> -	if_test ("binary search with binsearch works") {
> -		int haystack[] = { 2, 4, 6, 8, 10 };
> -		struct {
> -			int needle;
> -			size_t expected_idx;
> -		} testcases[] = {
> -			{-9000, 0},
> -			{-1, 0},
> -			{0, 0},
> -			{2, 0},
> -			{3, 1},
> -			{4, 1},
> -			{7, 3},
> -			{9, 4},
> -			{10, 4},
> -			{11, 5},
> -			{9000, 5},
> -		};
> -
> -		for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) {
> -			struct integer_needle_lesseq_args args = {
> -				.haystack = haystack,
> -				.needle = testcases[i].needle,
> -			};
> -			size_t idx;
> -
> -			idx = binsearch(ARRAY_SIZE(haystack),
> -					&integer_needle_lesseq, &args);
> -			check_int(idx, ==, testcases[i].expected_idx);
> -		}
> -	}
> -
> -	if_test ("names_length returns size of a NULL-terminated string array") {
> -		const char *a[] = { "a", "b", NULL };
> -		check_int(names_length(a), ==, 2);
> -	}
> -
> -	if_test ("names_equal compares NULL-terminated string arrays") {
> -		const char *a[] = { "a", "b", "c", NULL };
> -		const char *b[] = { "a", "b", "d", NULL };
> -		const char *c[] = { "a", "b", NULL };
> -
> -		check(names_equal(a, a));
> -		check(!names_equal(a, b));
> -		check(!names_equal(a, c));
> -	}
> -
> -	if_test ("parse_names works for basic input") {
> -		char in1[] = "line\n";
> -		char in2[] = "a\nb\nc";
> -		char **out = parse_names(in1, strlen(in1));
> -		check(out != NULL);
> -		check_str(out[0], "line");
> -		check(!out[1]);
> -		free_names(out);
> -
> -		out = parse_names(in2, strlen(in2));
> -		check(out != NULL);
> -		check_str(out[0], "a");
> -		check_str(out[1], "b");
> -		check_str(out[2], "c");
> -		check(!out[3]);
> -		free_names(out);
> -	}
> -
> -	if_test ("parse_names drops empty string") {
> -		char in[] = "a\n\nb\n";
> -		char **out = parse_names(in, strlen(in));
> -		check(out != NULL);
> -		check_str(out[0], "a");
> -		/* simply '\n' should be dropped as empty string */
> -		check_str(out[1], "b");
> -		check(!out[2]);
> -		free_names(out);
> -	}
> -
> -	if_test ("common_prefix_size works") {
> -		struct reftable_buf a = REFTABLE_BUF_INIT;
> -		struct reftable_buf b = REFTABLE_BUF_INIT;
> -		struct {
> -			const char *a, *b;
> -			int want;
> -		} cases[] = {
> -			{"abcdef", "abc", 3},
> -			{ "abc", "ab", 2 },
> -			{ "", "abc", 0 },
> -			{ "abc", "abd", 2 },
> -			{ "abc", "pqr", 0 },
> -		};
> -
> -		for (size_t i = 0; i < ARRAY_SIZE(cases); i++) {
> -			check(!reftable_buf_addstr(&a, cases[i].a));
> -			check(!reftable_buf_addstr(&b, cases[i].b));
> -			check_uint(common_prefix_size(&a, &b), ==, cases[i].want);
> -			reftable_buf_reset(&a);
> -			reftable_buf_reset(&b);
> -		}
> -		reftable_buf_release(&a);
> -		reftable_buf_release(&b);
> -	}
> -
> -	if_test ("reftable_put_be64 and reftable_get_be64 work") {
> -		uint64_t in = 0x1122334455667788;
> -		uint8_t dest[8];
> -		uint64_t out;
> -		reftable_put_be64(dest, in);
> -		out = reftable_get_be64(dest);
> -		check_int(in, ==, out);
> -	}
> -
> -	if_test ("reftable_put_be32 and reftable_get_be32 work") {
> -		uint32_t in = 0x11223344;
> -		uint8_t dest[4];
> -		uint32_t out;
> -		reftable_put_be32(dest, in);
> -		out = reftable_get_be32(dest);
> -		check_int(in, ==, out);
> -	}
> -
> -	if_test ("reftable_put_be24 and reftable_get_be24 work") {
> -		uint32_t in = 0x112233;
> -		uint8_t dest[3];
> -		uint32_t out;
> -		reftable_put_be24(dest, in);
> -		out = reftable_get_be24(dest);
> -		check_int(in, ==, out);
> -	}
> -
> -	if_test ("put_be16 and get_be16 work") {
> -		uint32_t in = 0xfef1;
> -		uint8_t dest[3];
> -		uint32_t out;
> -		reftable_put_be16(dest, in);
> -		out = reftable_get_be16(dest);
> -		check_int(in, ==, out);
> -	}
> -
> -	if_test ("REFTABLE_ALLOC_GROW works") {
> -		int *arr = NULL, *old_arr;
> -		size_t alloc = 0, old_alloc;
> -
> -		check(!REFTABLE_ALLOC_GROW(arr, 1, alloc));
> -		check(arr != NULL);
> -		check_uint(alloc, >=, 1);
> -		arr[0] = 42;
> -
> -		old_alloc = alloc;
> -		old_arr = arr;
> -		reftable_set_alloc(NULL, realloc_stub, NULL);
> -		check(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> -		check(arr == old_arr);
> -		check_uint(alloc, ==, old_alloc);
> -
> -		old_alloc = alloc;
> -		reftable_set_alloc(NULL, NULL, NULL);
> -		check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> -		check(arr != NULL);
> -		check_uint(alloc, >, old_alloc);
> -		arr[alloc - 1] = 42;
> -
> -		reftable_free(arr);
> -	}
> -
> -	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
> -		int *arr = NULL;
> -		size_t alloc = 0, old_alloc;
> -
> -		REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
> -		check(arr != NULL);
> -		check_uint(alloc, >=, 1);
> -		arr[0] = 42;
> -
> -		old_alloc = alloc;
> -		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> -		check(arr != NULL);
> -		check_uint(alloc, >, old_alloc);
> -		arr[alloc - 1] = 42;
> -
> -		old_alloc = alloc;
> -		reftable_set_alloc(NULL, realloc_stub, NULL);
> -		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> -		check(arr == NULL);
> -		check_uint(alloc, ==, 0);
> -		reftable_set_alloc(NULL, NULL, NULL);
> -
> -		reftable_free(arr);
> -	}
> -
> -	return test_done();
> -}
> diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
> new file mode 100644
> index 0000000000..63dd568faf
> --- /dev/null
> +++ b/t/unit-tests/u-reftable-basics.c
> @@ -0,0 +1,195 @@
> +/*
> +Copyright 2020 Google LLC
> +
> +Use of this source code is governed by a BSD-style
> +license that can be found in the LICENSE file or at
> +https://developers.google.com/open-source/licenses/bsd
> +*/
> +
> +#include "unit-test.h"
> +#include "reftable/basics.h"
> +
> +struct integer_needle_lesseq_args {
> +	int needle;
> +	int *haystack;
> +};
> +
> +static int integer_needle_lesseq(size_t i, void *_args)
> +{
> +	struct integer_needle_lesseq_args *args = _args;
> +	return args->needle <= args->haystack[i];
> +}
> +
> +static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
> +{
> +	return NULL;
> +}
> +
> +void test_reftable_basics__binsearch(void)
> +{
> +	int haystack[] = { 2, 4, 6, 8, 10 };
> +	struct {
> +		int needle;
> +		size_t expected_idx;
> +	} testcases[] = {
> +		{-9000, 0},
> +		{-1, 0},
> +		{0, 0},
> +		{2, 0},
> +		{3, 1},
> +		{4, 1},
> +		{7, 3},
> +		{9, 4},
> +		{10, 4},
> +		{11, 5},
> +		{9000, 5},
> +	};
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) {
> +		struct integer_needle_lesseq_args args = {
> +			.haystack = haystack,
> +			.needle = testcases[i].needle,
> +		};
> +		size_t idx;
> +
> +		idx = binsearch(ARRAY_SIZE(haystack),
> +				&integer_needle_lesseq, &args);
> +		cl_assert_equal_i(idx, testcases[i].expected_idx);
> +	}
> +

Nit: empty newline can be removed.

> +}
> +
> +void test_reftable_basics__names_length(void)
> +{
> +	const char *a[] = { "a", "b", NULL };
> +	cl_assert_equal_i(names_length(a), 2);
> +}
> +
> +void test_reftable_basics__names_equal(void)
> +{
> +	const char *a[] = { "a", "b", "c", NULL };
> +	const char *b[] = { "a", "b", "d", NULL };
> +	const char *c[] = { "a", "b", NULL };
> +
> +	cl_assert(names_equal(a, a));
> +	cl_assert(!names_equal(a, b));
> +	cl_assert(!names_equal(a, c));
> +}
> +
> +void test_reftable_basics__parse_names(void)
> +{
> +	char in1[] = "line\n";
> +	char in2[] = "a\nb\nc";
> +	char **out = parse_names(in1, strlen(in1));
> +	cl_assert(out != NULL);
> +	cl_assert_equal_s(out[0], "line");
> +	cl_assert(!out[1]);
> +	free_names(out);
> +
> +	out = parse_names(in2, strlen(in2));
> +	cl_assert(out != NULL);
> +	cl_assert_equal_s(out[0], "a");
> +	cl_assert_equal_s(out[1], "b");
> +	cl_assert_equal_s(out[2], "c");
> +	cl_assert(!out[3]);
> +	free_names(out);
> +}
> +

I think you missed converting "parse_names drops empty string".

> +void test_reftable_basics__common_prefix_size(void)
> +{
> +	struct reftable_buf a = REFTABLE_BUF_INIT;
> +	struct reftable_buf b = REFTABLE_BUF_INIT;
> +	struct {
> +		const char *a, *b;
> +		int want;
> +	} cases[] = {
> +		{"abcdef", "abc", 3},
> +		{ "abc", "ab", 2 },
> +		{ "", "abc", 0 },
> +		{ "abc", "abd", 2 },
> +		{ "abc", "pqr", 0 },
> +	};
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(cases); i++) {
> +		reftable_buf_reset(&a);
> +		reftable_buf_reset(&b);
> +		cl_assert_equal_i(reftable_buf_addstr(&a, cases[i].a), 0);
> +		cl_assert_equal_i(reftable_buf_addstr(&b, cases[i].b), 0);
> +		cl_assert_equal_i(common_prefix_size(&a, &b), cases[i].want);

Why did you change the order of `reftable_buf_reset()` calls? The
reordered logic achieves the same result, but I'd recommend to keep
things as-is so that reviewers aren't puzzled by this arbitrary change.

> +	}
> +	reftable_buf_release(&a);
> +	reftable_buf_release(&b);
> +}

I miss tests for `put_be64` and `put_be32`.

> +void test_reftable_basics__put_get_be24(void)
> +{
> +	uint32_t in = 0x112233;
> +	uint8_t dest[3];
> +	uint32_t out;
> +	reftable_put_be24(dest, in);
> +	out = reftable_get_be24(dest);
> +	cl_assert_equal_i(in, out);
> +}
> +
> +void test_reftable_basics__put_get_be16(void)
> +{
> +	uint32_t in = 0xfef1;
> +	uint8_t dest[3];
> +	uint32_t out;
> +	reftable_put_be16(dest, in);
> +	out = reftable_get_be16(dest);
> +	cl_assert_equal_i(in, out);
> +}
> +
> +void test_reftable_basics__grow_alloc(void)

Let's rename this to `__alloc_grow()` to match the name of the function.

> +{
> +	int *arr = NULL, *old_arr;
> +	size_t alloc = 0, old_alloc;
> +
> +	cl_assert_equal_i(REFTABLE_ALLOC_GROW(arr, 1, alloc), 0);
> +	cl_assert(arr != NULL);
> +	cl_assert(alloc >= 1);
> +	arr[0] = 42;
> +
> +	old_alloc = alloc;
> +	old_arr = arr;
> +	reftable_set_alloc(NULL, realloc_stub, NULL);
> +	cl_assert(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> +	cl_assert(arr == old_arr);
> +	cl_assert_equal_i(alloc, old_alloc);
> +
> +	old_alloc = alloc;
> +	reftable_set_alloc(NULL, NULL, NULL);
> +	cl_assert_equal_i(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc), 0);
> +	cl_assert(arr != NULL);
> +	cl_assert(alloc > old_alloc);
> +	arr[alloc - 1] = 42;
> +
> +	reftable_free(arr);
> +}
> +
> +void test_reftable_basics__grow_alloc_or_null(void)

Same here, let's rename to `alloc_grow_or_null`.

> +{
> +	int *arr = NULL;
> +	size_t alloc = 0, old_alloc;
> +
> +	REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
> +	cl_assert(arr != NULL);
> +	cl_assert(alloc >= 1);
> +	arr[0] = 42;
> +
> +	old_alloc = alloc;
> +	REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> +	cl_assert(arr != NULL);
> +	cl_assert(alloc > old_alloc);
> +	arr[alloc - 1] = 42;
> +
> +	old_alloc = alloc;
> +	reftable_set_alloc(NULL, realloc_stub, NULL);
> +	REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> +	cl_assert(arr == NULL);
> +	cl_assert_equal_i(alloc, 0);
> +	reftable_set_alloc(NULL, NULL, NULL);
> +
> +	reftable_free(arr);
> +}

Other than that this looks well-done to me, thanks!

Patrick