[PATCH 0/5] t/unit-tests: convert unit-tests to use clar

[PATCH 0/5] t/unit-tests: convert unit-tests to use clar

[Seyi Kuforiji]

Hello,

This small patch series transitions a couple more of our existing unit
test files to the Clar testing framework. This change is part of our
ongoing effort to standardize our testing framework to enhance
maintainability.

Thanks
Seyi

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>

Seyi Kuforiji (5):
  t/unit-tests: implement oid helper functions in unit-tests.{c,h}
  t/unit-tests: convert oid-array test to use clar test framework
  t/unit-tests: convert oidmap test to use clar test framework
  t/unit-tests: convert oidtree test to use clar test framework
  t/unit-tests: remove lib-oid.{c,h,o}

 Makefile                                      |   7 +-
 t/meson.build                                 |   7 +-
 t/unit-tests/lib-oid.c                        |  52 ------
 t/unit-tests/lib-oid.h                        |  25 ---
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 123 +++++++-------
 t/unit-tests/{t-oidmap.c => u-oidmap.c}       | 153 ++++++------------
 t/unit-tests/{t-oidtree.c => u-oidtree.c}     |  78 ++++-----
 t/unit-tests/unit-test.c                      |  42 +++++
 t/unit-tests/unit-test.h                      |  19 +++
 9 files changed, 212 insertions(+), 294 deletions(-)
 delete mode 100644 t/unit-tests/lib-oid.c
 delete mode 100644 t/unit-tests/lib-oid.h
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (35%)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (44%)

-- 
2.47.0.86.g15030f9556

[PATCH 1/5] t/unit-tests: implement oid helper functions in unit-tests.{c,h}

[Seyi Kuforiji]

`get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
oid-related tests to run without errors. In the current implementation,
both functions are defined and declared in the
`t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
the homegrown unit tests structure.

Implement equivalent functions in unit-tests.{c,h}. Both these functions
become available for oid-related test files implemented using the clar
testing framework, which requires them. This will be used by subsequent
commits.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 t/unit-tests/unit-test.c | 42 ++++++++++++++++++++++++++++++++++++++++
 t/unit-tests/unit-test.h | 19 ++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
index fa8818842a..13d54f192a 100644
--- a/t/unit-tests/unit-test.c
+++ b/t/unit-tests/unit-test.c
@@ -1,5 +1,7 @@
 #include "unit-test.h"
+#include "hex.h"
 #include "parse-options.h"
+#include "strbuf.h"
 #include "string-list.h"
 #include "strvec.h"
 
@@ -62,3 +64,43 @@ int cmd_main(int argc, const char **argv)
 	strvec_clear(&args);
 	return ret;
 }
+
+int init_hash_algo(void)
+{
+	static int algo = -1;
+
+	if (algo < 0) {
+		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
+		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
+
+		cl_assert(algo != GIT_HASH_UNKNOWN);
+	}
+	return algo;
+}
+
+static void cl_parse_oid(const char *hex, struct object_id *oid,
+				       const struct git_hash_algo *algop)
+{
+	int ret;
+	size_t sz = strlen(hex);
+	struct strbuf buf = STRBUF_INIT;
+
+	cl_assert(sz <= algop->hexsz);
+
+	strbuf_add(&buf, hex, sz);
+	strbuf_addchars(&buf, '0', algop->hexsz - sz);
+
+	ret = get_oid_hex_algop(buf.buf, oid, algop);
+	cl_assert_equal_i(ret, 0);
+
+	strbuf_release(&buf);
+}
+
+
+void cl_parse_any_oid(const char *hex, struct object_id *oid)
+{
+	int hash_algo = init_hash_algo();
+
+	cl_assert(hash_algo != GIT_HASH_UNKNOWN);
+	cl_parse_oid(hex, oid, &hash_algos[hash_algo]);
+}
diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
index 85e5d6a948..ebed51212f 100644
--- a/t/unit-tests/unit-test.h
+++ b/t/unit-tests/unit-test.h
@@ -8,3 +8,22 @@
 	snprintf(desc, sizeof(desc), fmt, __VA_ARGS__); \
 	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
 } while (0)
+
+/*
+ * Convert arbitrary hex string to object_id.
+ * For example, passing "abc12" will generate
+ * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
+ * create object_id with that.
+ * WARNING: passing a string of length more than the hexsz of respective hash
+ * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
+ * environment variable.
+ */
+void cl_parse_any_oid(const char *s, struct object_id *oid);
+/*
+ * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
+ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
+ * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
+ * cl_assert(algo != GIT_HASH_UNKNOWN) before returning to verify if the
+ * GIT_TEST_DEFAULT_HASH's value is valid or not.
+ */
+int init_hash_algo(void);
-- 
2.47.0.86.g15030f9556

[PATCH 2/5] t/unit-tests: convert oid-array test to use clar

[Seyi Kuforiji]

Adapt oid-array test script to clar framework by using clar assertions
where necessary. Remove descriptions from macros to reduce
redundancy, and move test input arrays to global scope for reuse across
multiple test functions. Introduce `test_oid_array__initialize()` to
explicitly initialize the hash algorithm.

These changes streamline the test suite, making individual tests
self-contained and reducing redundant code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                      |   2 +-
 t/meson.build                                 |   2 +-
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 123 +++++++++---------
 3 files changed, 65 insertions(+), 62 deletions(-)
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (35%)

diff --git a/Makefile b/Makefile
index bcf5ed3f85..f8e061365b 100644
--- a/Makefile
+++ b/Makefile
@@ -1356,6 +1356,7 @@ CLAR_TEST_SUITES += u-example-decorate
 CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
+CLAR_TEST_SUITES += u-oid-array
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1366,7 +1367,6 @@ CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
-UNIT_TEST_PROGRAMS += t-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
diff --git a/t/meson.build b/t/meson.build
index 780939d49f..93410a8545 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -4,6 +4,7 @@ clar_test_suites = [
   'unit-tests/u-hash.c',
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
+  'unit-tests/u-oid-array.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -48,7 +49,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/u-oid-array.c
similarity index 35%
rename from t/unit-tests/t-oid-array.c
rename to t/unit-tests/u-oid-array.c
index 45b59a2a51..6d173dc004 100644
--- a/t/unit-tests/t-oid-array.c
+++ b/t/unit-tests/u-oid-array.c
@@ -1,22 +1,18 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
-#include "test-lib.h"
-#include "lib-oid.h"
+#include "unit-test.h"
 #include "oid-array.h"
 #include "hex.h"
 
-static int fill_array(struct oid_array *array, const char *hexes[], size_t n)
+static void fill_array(struct oid_array *array, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
 
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oid_array_append(array, &oid);
 	}
-	if (!check_uint(array->nr, ==, n))
-		return -1;
-	return 0;
+	cl_assert_equal_i(array->nr, n);
 }
 
 static int add_to_oid_array(const struct object_id *oid, void *data)
@@ -34,30 +30,22 @@ static void t_enumeration(const char **input_args, size_t input_sz,
 			 actual = OID_ARRAY_INIT;
 	size_t i;
 
-	if (fill_array(&input, input_args, input_sz))
-		return;
-	if (fill_array(&expect, expect_args, expect_sz))
-		return;
+	fill_array(&input, input_args, input_sz);
+	fill_array(&expect, expect_args, expect_sz);
 
 	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
-	if (!check_uint(actual.nr, ==, expect.nr))
-		return;
-
-	for (i = 0; i < actual.nr; i++) {
-		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
-			test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
-				 oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
-				 (uintmax_t)i);
-	}
+	cl_assert_equal_i(actual.nr, expect.nr);
+
+	for (i = 0; i < actual.nr; i++)
+		cl_assert(oideq(&actual.oid[i], &expect.oid[i]));
 
 	oid_array_clear(&actual);
 	oid_array_clear(&input);
 	oid_array_clear(&expect);
 }
 
-#define TEST_ENUMERATION(input, expect, desc)                                     \
-	TEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \
-			   desc " works")
+#define TEST_ENUMERATION(input, expect)                                     \
+	t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect));
 
 static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 		     int lower_bound, int upper_bound)
@@ -66,61 +54,76 @@ static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 	struct object_id oid_query;
 	int ret;
 
-	if (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))
-		return;
-	if (fill_array(&array, input_hexes, n))
-		return;
+	cl_parse_any_oid(query_hex, &oid_query);
+	fill_array(&array, input_hexes, n);
 	ret = oid_array_lookup(&array, &oid_query);
 
-	if (!check_int(ret, <=, upper_bound) ||
-	    !check_int(ret, >=, lower_bound))
-		test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
+	cl_assert(ret <= upper_bound);
+	cl_assert(ret >= lower_bound);
 
 	oid_array_clear(&array);
 }
 
-#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \
-	TEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
-		      lower_bound, upper_bound),                        \
-	     desc " works")
+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound) \
+	t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
+		      lower_bound, upper_bound);
 
-static void setup(void)
+void test_oid_array__initialize(void)
 {
 	/* The hash algo is used by oid_array_lookup() internally */
 	int algo = init_hash_algo();
-	if (check_int(algo, !=, GIT_HASH_UNKNOWN))
-		repo_set_hash_algo(the_repository, algo);
+	cl_assert(algo != GIT_HASH_UNKNOWN);
+	repo_set_hash_algo(the_repository, algo);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+static const char *arr_input[] = { "88", "44", "aa", "55" };
+static const char *arr_input_dup[] = { "88", "44", "aa", "55",
+				       "88", "44", "aa", "55",
+				       "88", "44", "aa", "55" };
+static const char *res_sorted[] = { "44", "55", "88", "aa" };
+
+void test_oid_array__enumerate_unique(void)
 {
-	const char *arr_input[] = { "88", "44", "aa", "55" };
-	const char *arr_input_dup[] = { "88", "44", "aa", "55",
-					"88", "44", "aa", "55",
-					"88", "44", "aa", "55" };
-	const char *res_sorted[] = { "44", "55", "88", "aa" };
-	const char *nearly_55;
+	TEST_ENUMERATION(arr_input, res_sorted);
+}
+
+void test_oid_array__enumerate_duplicate(void)
+{
+	TEST_ENUMERATION(arr_input_dup, res_sorted);
+}
+
+void test_oid_array__lookup(void)
+{
+	TEST_LOOKUP(arr_input, "55", 1, 1);
+}
 
-	if (!TEST(setup(), "setup"))
-		test_skip_all("hash algo initialization failed");
+void test_oid_array__lookup_non_existent(void)
+{
+	TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
+}
+
+void test_oid_array__lookup_duplicates(void)
+{
+	TEST_LOOKUP(arr_input_dup, "55", 3, 5);
+}
 
-	TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
-	TEST_ENUMERATION(arr_input_dup, res_sorted,
-			 "ordered enumeration with duplicate suppression");
+void test_oid_array__lookup_non_existent_dup(void)
+{
+	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
+}
 
-	TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
-	TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
-	TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
-	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
-		    "lookup non-existent entry with duplicates");
+void test_oid_array__lookup_almost_dup(void)
+{
+	const char *nearly_55;
 
 	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
 			"5500000000000000000000000000000000000001" :
 			"5500000000000000000000000000000000000000000000000000000000000001";
-	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
-		    "lookup with almost duplicate values");
-	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
-		    "lookup with single duplicate value");
 
-	return test_done();
+	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
+}
+
+void test_oid_array__lookup_single_dup(void)
+{
+	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
 }
-- 
2.47.0.86.g15030f9556

[PATCH 3/5] t/unit-tests: convert oidmap test to use clar

[Seyi Kuforiji]

Adapt oidmap test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid` handles the necessary checks needed
for the test to run smoothly.

Introduce 'test_oidmap__initialize` handles the to set up of the global
oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
frees the oidmap and its entries when all tests are completed.

This streamlines the test suite, making individual tests self-contained
and reducing redundant code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                |   2 +-
 t/meson.build                           |   2 +-
 t/unit-tests/{t-oidmap.c => u-oidmap.c} | 153 ++++++++----------------
 3 files changed, 54 insertions(+), 103 deletions(-)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)

diff --git a/Makefile b/Makefile
index f8e061365b..58a6af1eb0 100644
--- a/Makefile
+++ b/Makefile
@@ -1357,6 +1357,7 @@ CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-oid-array
+CLAR_TEST_SUITES += u-oidmap
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1367,7 +1368,6 @@ CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
-UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
diff --git a/t/meson.build b/t/meson.build
index 93410a8545..f9e0ae15df 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -5,6 +5,7 @@ clar_test_suites = [
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
+  'unit-tests/u-oidmap.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -49,7 +50,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/u-oidmap.c
similarity index 32%
rename from t/unit-tests/t-oidmap.c
rename to t/unit-tests/u-oidmap.c
index b22e52d08b..e40740db5c 100644
--- a/t/unit-tests/t-oidmap.c
+++ b/t/unit-tests/u-oidmap.c
@@ -1,5 +1,4 @@
-#include "test-lib.h"
-#include "lib-oid.h"
+#include "unit-test.h"
 #include "oidmap.h"
 #include "hash.h"
 #include "hex.h"
@@ -18,102 +17,85 @@ static const char *const key_val[][2] = { { "11", "one" },
 					  { "22", "two" },
 					  { "33", "three" } };
 
-static void setup(void (*f)(struct oidmap *map))
+static struct oidmap map;
+
+void test_oidmap__initialize(void)
 {
-	struct oidmap map = OIDMAP_INIT;
-	int ret = 0;
+	oidmap_init(&map, 0);
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++){
 		struct test_entry *entry;
 
 		FLEX_ALLOC_STR(entry, name, key_val[i][1]);
-		if ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) {
-			free(entry);
-			break;
-		}
-		entry = oidmap_put(&map, entry);
-		if (!check(entry == NULL))
-			free(entry);
+		cl_parse_any_oid(key_val[i][0], &entry->entry.oid);
+		cl_assert(oidmap_put(&map, entry) == NULL);
 	}
+}
 
-	if (!ret)
-		f(&map);
+void test_oidmap__cleanup(void)
+{
 	oidmap_free(&map, 1);
 }
 
-static void t_replace(struct oidmap *map)
+void test_oidmap__replace(void)
 {
 	struct test_entry *entry, *prev;
 
 	FLEX_ALLOC_STR(entry, name, "un");
-	if (get_oid_arbitrary_hex("11", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "one");
+	cl_parse_any_oid("11", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "one");
 	free(prev);
 
 	FLEX_ALLOC_STR(entry, name, "deux");
-	if (get_oid_arbitrary_hex("22", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "two");
+	cl_parse_any_oid("22", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "two");
 	free(prev);
 }
 
-static void t_get(struct oidmap *map)
+void test_oidmap__get(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_get(map, &oid) == NULL);
-
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_get(&map, &oid) == NULL);
+
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
 }
 
-static void t_remove(struct oidmap *map)
+void test_oidmap__remove(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_remove(map, &oid) == NULL);
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_remove(&map, &oid) == NULL);
 }
 
 static int key_val_contains(struct test_entry *entry, char seen[])
@@ -121,8 +103,7 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		struct object_id oid;
 
-		if (get_oid_arbitrary_hex(key_val[i][0], &oid))
-			return -1;
+		cl_parse_any_oid(key_val[i][0], &oid);
 
 		if (oideq(&entry->entry.oid, &oid)) {
 			if (seen[i])
@@ -134,48 +115,18 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	return 1;
 }
 
-static void t_iterate(struct oidmap *map)
+void test_oidmap__iterate(void)
 {
 	struct oidmap_iter iter;
 	struct test_entry *entry;
 	char seen[ARRAY_SIZE(key_val)] = { 0 };
 	int count = 0;
 
-	oidmap_iter_init(map, &iter);
+	oidmap_iter_init(&map, &iter);
 	while ((entry = oidmap_iter_next(&iter))) {
-		int ret;
-		if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {
-			switch (ret) {
-			case -1:
-				break; /* error message handled by get_oid_arbitrary_hex() */
-			case 1:
-				test_msg("obtained entry was not given in the input\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			case 2:
-				test_msg("duplicate entry detected\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			default:
-				test_msg("BUG: invalid return value (%d) from key_val_contains()",
-					 ret);
-				break;
-			}
-		} else {
-			count++;
-		}
+		cl_assert_equal_i(key_val_contains(entry, seen), 0);
+		count++;
 	}
-	check_int(count, ==, ARRAY_SIZE(key_val));
-	check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));
-}
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST(setup(t_replace), "replace works");
-	TEST(setup(t_get), "get works");
-	TEST(setup(t_remove), "remove works");
-	TEST(setup(t_iterate), "iterate works");
-	return test_done();
+	cl_assert_equal_i(count, ARRAY_SIZE(key_val));
+	cl_assert_equal_i(hashmap_get_size(&map.map), ARRAY_SIZE(key_val));
 }
-- 
2.47.0.86.g15030f9556

[PATCH 4/5] t/unit-tests: convert oidtree test to use clar

[Seyi Kuforiji]

Adapt oidtree test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid` handles the necessary checks needed
for the test to run smoothly.

Introduce 'test_oidtree__initialize` handles the to set up of the global
oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
tests are completed.

This streamlines the test suite, making individual tests self-contained
and reducing redundant code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                  |  2 +-
 t/meson.build                             |  2 +-
 t/unit-tests/{t-oidtree.c => u-oidtree.c} | 78 +++++++++--------------
 3 files changed, 32 insertions(+), 50 deletions(-)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (44%)

diff --git a/Makefile b/Makefile
index 58a6af1eb0..feb01702c7 100644
--- a/Makefile
+++ b/Makefile
@@ -1358,6 +1358,7 @@ CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 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-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1368,7 +1369,6 @@ CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
-UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
diff --git a/t/meson.build b/t/meson.build
index f9e0ae15df..0b412a7c16 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -6,6 +6,7 @@ clar_test_suites = [
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
   'unit-tests/u-oidmap.c',
+  'unit-tests/u-oidtree.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -50,7 +51,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
   'unit-tests/t-reftable-merged.c',
diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/u-oidtree.c
similarity index 44%
rename from t/unit-tests/t-oidtree.c
rename to t/unit-tests/u-oidtree.c
index a38754b066..de6f6bd292 100644
--- a/t/unit-tests/t-oidtree.c
+++ b/t/unit-tests/u-oidtree.c
@@ -1,10 +1,11 @@
-#include "test-lib.h"
-#include "lib-oid.h"
+#include "unit-test.h"
 #include "oidtree.h"
 #include "hash.h"
 #include "hex.h"
 #include "strvec.h"
 
+static struct oidtree ot;
+
 #define FILL_TREE(tree, ...)                                       \
 	do {                                                       \
 		const char *hexes[] = { __VA_ARGS__ };             \
@@ -16,8 +17,7 @@ static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oidtree_insert(ot, &oid);
 	}
 	return 0;
@@ -27,10 +27,8 @@ static void check_contains(struct oidtree *ot, const char *hex, int expected)
 {
 	struct object_id oid;
 
-	if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
-		return;
-	if (!check_int(oidtree_contains(ot, &oid), ==, expected))
-		test_msg("oid: %s", oid_to_hex(&oid));
+	cl_parse_any_oid(hex, &oid);
+	cl_assert_equal_i(oidtree_contains(ot, &oid), expected);
 }
 
 struct expected_hex_iter {
@@ -44,19 +42,11 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)
 	struct expected_hex_iter *hex_iter = data;
 	struct object_id expected;
 
-	if (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {
-		test_msg("error: extraneous callback for query: ('%s'), object_id: ('%s')",
-			 hex_iter->query, oid_to_hex(oid));
-		return CB_BREAK;
-	}
-
-	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
-					     &expected), ==, 0))
-		; /* the data is bogus and cannot be used */
-	else if (!check(oideq(oid, &expected)))
-		test_msg("expected: %s\n       got: %s\n     query: %s",
-			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
+	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
 
+	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
+			 &expected);
+	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));
 	hex_iter->i += 1;
 	return CB_CONTINUE;
 }
@@ -75,48 +65,40 @@ static void check_each(struct oidtree *ot, const char *query, ...)
 		strvec_push(&hex_iter.expected_hexes, arg);
 	va_end(hex_args);
 
-	if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
-		return;
+	cl_parse_any_oid(query, &oid);
 	oidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);
 
-	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
-		test_msg("error: could not find some 'object_id's for query ('%s')", query);
+	cl_assert_equal_i(hex_iter.i, hex_iter.expected_hexes.nr);
 	strvec_clear(&hex_iter.expected_hexes);
 }
 
-static void setup(void (*f)(struct oidtree *ot))
+void test_oidtree__initialize(void)
 {
-	struct oidtree ot;
-
 	oidtree_init(&ot);
-	f(&ot);
-	oidtree_clear(&ot);
 }
 
-static void t_contains(struct oidtree *ot)
+void test_oidtree__cleanup(void)
 {
-	FILL_TREE(ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
-	check_contains(ot, "44", 0);
-	check_contains(ot, "441", 0);
-	check_contains(ot, "440", 0);
-	check_contains(ot, "444", 1);
-	check_contains(ot, "4440", 1);
-	check_contains(ot, "4444", 0);
+	oidtree_clear(&ot);
 }
 
-static void t_each(struct oidtree *ot)
+void test_oidtree__contains(void)
 {
-	FILL_TREE(ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
-	check_each(ot, "12300", "123", NULL);
-	check_each(ot, "3211", NULL); /* should not reach callback */
-	check_each(ot, "3210", "321", NULL);
-	check_each(ot, "32100", "321", NULL);
-	check_each(ot, "32", "320", "321", NULL);
+	FILL_TREE(&ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
+	check_contains(&ot, "44", 0);
+	check_contains(&ot, "441", 0);
+	check_contains(&ot, "440", 0);
+	check_contains(&ot, "444", 1);
+	check_contains(&ot, "4440", 1);
+	check_contains(&ot, "4444", 0);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_oidtree__each(void)
 {
-	TEST(setup(t_contains), "oidtree insert and contains works");
-	TEST(setup(t_each), "oidtree each works");
-	return test_done();
+	FILL_TREE(&ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
+	check_each(&ot, "12300", "123", NULL);
+	check_each(&ot, "3211", NULL); /* should not reach callback */
+	check_each(&ot, "3210", "321", NULL);
+	check_each(&ot, "32100", "321", NULL);
+	check_each(&ot, "32", "320", "321", NULL);
 }
-- 
2.47.0.86.g15030f9556

[PATCH 5/5] t/unit-tests: remove lib-oid.{c,h,o}

[Seyi Kuforiji]

The `lib-oid.c`, `lib-oid.h`, and `lib-oid.o files` are no longer needed
since their equivalent functions have been implemented in unit-test.c
and unit-test.h. This removes redundant code and ensures all unit
test-related functionality is consolidated in a single location.

Drop references to lib-oid from our `Makefile`, and `meson.build` files
to prevent build errors due to missing files.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile               |  1 -
 t/meson.build          |  1 -
 t/unit-tests/lib-oid.c | 52 ------------------------------------------
 t/unit-tests/lib-oid.h | 25 --------------------
 4 files changed, 79 deletions(-)
 delete mode 100644 t/unit-tests/lib-oid.c
 delete mode 100644 t/unit-tests/lib-oid.h

diff --git a/Makefile b/Makefile
index feb01702c7..6afa6587ba 100644
--- a/Makefile
+++ b/Makefile
@@ -1381,7 +1381,6 @@ UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
-UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
diff --git a/t/meson.build b/t/meson.build
index 0b412a7c16..c1c4aa32aa 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -68,7 +68,6 @@ foreach unit_test_program : unit_test_programs
   unit_test = executable(unit_test_name,
     sources: [
       'unit-tests/test-lib.c',
-      'unit-tests/lib-oid.c',
       'unit-tests/lib-reftable.c',
       unit_test_program,
     ],
diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c
deleted file mode 100644
index 8f0ccac532..0000000000
--- a/t/unit-tests/lib-oid.c
+++ /dev/null
@@ -1,52 +0,0 @@
-#include "test-lib.h"
-#include "lib-oid.h"
-#include "strbuf.h"
-#include "hex.h"
-
-int init_hash_algo(void)
-{
-	static int algo = -1;
-
-	if (algo < 0) {
-		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
-		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
-
-		if (!check(algo != GIT_HASH_UNKNOWN))
-			test_msg("BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')",
-				 algo_name);
-	}
-	return algo;
-}
-
-static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
-				       const struct git_hash_algo *algop)
-{
-	int ret;
-	size_t sz = strlen(hex);
-	struct strbuf buf = STRBUF_INIT;
-
-	if (!check(sz <= algop->hexsz)) {
-		test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
-			 hex, (unsigned long)algop->hexsz);
-		return -1;
-	}
-
-	strbuf_add(&buf, hex, sz);
-	strbuf_addchars(&buf, '0', algop->hexsz - sz);
-
-	ret = get_oid_hex_algop(buf.buf, oid, algop);
-	if (!check_int(ret, ==, 0))
-		test_msg("BUG: invalid hex input (%s) provided", hex);
-
-	strbuf_release(&buf);
-	return ret;
-}
-
-int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)
-{
-	int hash_algo = init_hash_algo();
-
-	if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))
-		return -1;
-	return get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);
-}
diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
deleted file mode 100644
index 4e77c04bd2..0000000000
--- a/t/unit-tests/lib-oid.h
+++ /dev/null
@@ -1,25 +0,0 @@
-#ifndef LIB_OID_H
-#define LIB_OID_H
-
-#include "hash.h"
-
-/*
- * Convert arbitrary hex string to object_id.
- * For example, passing "abc12" will generate
- * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
- * create object_id with that.
- * WARNING: passing a string of length more than the hexsz of respective hash
- * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
- * environment variable.
- */
-int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
-/*
- * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
- * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
- * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
- * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the
- * GIT_TEST_DEFAULT_HASH's value is valid or not.
- */
-int init_hash_algo(void);
-
-#endif /* LIB_OID_H */
-- 
2.47.0.86.g15030f9556

Re: [PATCH 1/5] t/unit-tests: implement oid helper functions in unit-tests.{c,h}

[Phillip Wood]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> `get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
> oid-related tests to run without errors. In the current implementation,
> both functions are defined and declared in the
> `t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
> the homegrown unit tests structure.
> 
> Implement equivalent functions in unit-tests.{c,h}. Both these functions
> become available for oid-related test files implemented using the clar
> testing framework, which requires them. This will be used by subsequent
> commits.

It is nice to see these tests being moved over to clar but I'm not sure 
that moving these functions into this file is good idea. All the unit 
tests need to link against unit-tests.o but only a subset will want 
access to these functions. Putting them in this file means that all the 
tests will now depend on code from strbuf.o and hex.o. I think we could 
add the new functions to lib-oid.c and then remove the old ones when 
there are not needed any more.

Best Wishes

Phillip

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>   t/unit-tests/unit-test.c | 42 ++++++++++++++++++++++++++++++++++++++++
>   t/unit-tests/unit-test.h | 19 ++++++++++++++++++
>   2 files changed, 61 insertions(+)
> 
> diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
> index fa8818842a..13d54f192a 100644
> --- a/t/unit-tests/unit-test.c
> +++ b/t/unit-tests/unit-test.c
> @@ -1,5 +1,7 @@
>   #include "unit-test.h"
> +#include "hex.h"
>   #include "parse-options.h"
> +#include "strbuf.h"
>   #include "string-list.h"
>   #include "strvec.h"
>   
> @@ -62,3 +64,43 @@ int cmd_main(int argc, const char **argv)
>   	strvec_clear(&args);
>   	return ret;
>   }
> +
> +int init_hash_algo(void)
> +{
> +	static int algo = -1;
> +
> +	if (algo < 0) {
> +		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
> +		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
> +
> +		cl_assert(algo != GIT_HASH_UNKNOWN);
> +	}
> +	return algo;
> +}
> +
> +static void cl_parse_oid(const char *hex, struct object_id *oid,
> +				       const struct git_hash_algo *algop)
> +{
> +	int ret;
> +	size_t sz = strlen(hex);
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	cl_assert(sz <= algop->hexsz);
> +
> +	strbuf_add(&buf, hex, sz);
> +	strbuf_addchars(&buf, '0', algop->hexsz - sz);
> +
> +	ret = get_oid_hex_algop(buf.buf, oid, algop);
> +	cl_assert_equal_i(ret, 0);
> +
> +	strbuf_release(&buf);
> +}
> +
> +
> +void cl_parse_any_oid(const char *hex, struct object_id *oid)
> +{
> +	int hash_algo = init_hash_algo();
> +
> +	cl_assert(hash_algo != GIT_HASH_UNKNOWN);
> +	cl_parse_oid(hex, oid, &hash_algos[hash_algo]);
> +}
> diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
> index 85e5d6a948..ebed51212f 100644
> --- a/t/unit-tests/unit-test.h
> +++ b/t/unit-tests/unit-test.h
> @@ -8,3 +8,22 @@
>   	snprintf(desc, sizeof(desc), fmt, __VA_ARGS__); \
>   	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
>   } while (0)
> +
> +/*
> + * Convert arbitrary hex string to object_id.
> + * For example, passing "abc12" will generate
> + * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
> + * create object_id with that.
> + * WARNING: passing a string of length more than the hexsz of respective hash
> + * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
> + * environment variable.
> + */
> +void cl_parse_any_oid(const char *s, struct object_id *oid);
> +/*
> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
> + * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
> + * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
> + * cl_assert(algo != GIT_HASH_UNKNOWN) before returning to verify if the
> + * GIT_TEST_DEFAULT_HASH's value is valid or not.
> + */
> +int init_hash_algo(void);

Re: [PATCH 2/5] t/unit-tests: convert oid-array test to use clar

[Phillip Wood]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Adapt oid-array test script to clar framework by using clar assertions
> where necessary. Remove descriptions from macros to reduce
> redundancy, and move test input arrays to global scope for reuse across
> multiple test functions. Introduce `test_oid_array__initialize()` to
> explicitly initialize the hash algorithm.
> 
> These changes streamline the test suite, making individual tests
> self-contained and reducing redundant code.

I think these conversion look correct but once again we're losing 
valuable debugging information because we haven't added better 
assertions to clar.

>   	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> -	if (!check_uint(actual.nr, ==, expect.nr))
> -		return;
> -
> -	for (i = 0; i < actual.nr; i++) {
> -		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
> -			test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
> -				 oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
> -				 (uintmax_t)i);
> -	}
> +	cl_assert_equal_i(actual.nr, expect.nr);
> +
> +	for (i = 0; i < actual.nr; i++)
> +		cl_assert(oideq(&actual.oid[i], &expect.oid[i]));

If this fails the poor person debugging it will have no idea why as 
there is now no indication of which two oids were being compared.

> -	if (!check_int(ret, <=, upper_bound) ||
> -	    !check_int(ret, >=, lower_bound))
> -		test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
> +	cl_assert(ret <= upper_bound);
> +	cl_assert(ret >= lower_bound);

This is another case where we could do with better assertions in clar

> -static void setup(void)
> +void test_oid_array__initialize(void)
>   {
>   	/* The hash algo is used by oid_array_lookup() internally */
>   	int algo = init_hash_algo();
> -	if (check_int(algo, !=, GIT_HASH_UNKNOWN))
> -		repo_set_hash_algo(the_repository, algo);
> +	cl_assert(algo != GIT_HASH_UNKNOWN);

init_has_algo() in unit-test.c already does this.

Best Wishes

Phillip


> +	repo_set_hash_algo(the_repository, algo);
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +static const char *arr_input[] = { "88", "44", "aa", "55" };
> +static const char *arr_input_dup[] = { "88", "44", "aa", "55",
> +				       "88", "44", "aa", "55",
> +				       "88", "44", "aa", "55" };
> +static const char *res_sorted[] = { "44", "55", "88", "aa" };
> +
> +void test_oid_array__enumerate_unique(void)
>   {
> -	const char *arr_input[] = { "88", "44", "aa", "55" };
> -	const char *arr_input_dup[] = { "88", "44", "aa", "55",
> -					"88", "44", "aa", "55",
> -					"88", "44", "aa", "55" };
> -	const char *res_sorted[] = { "44", "55", "88", "aa" };
> -	const char *nearly_55;
> +	TEST_ENUMERATION(arr_input, res_sorted);
> +}
> +
> +void test_oid_array__enumerate_duplicate(void)
> +{
> +	TEST_ENUMERATION(arr_input_dup, res_sorted);
> +}
> +
> +void test_oid_array__lookup(void)
> +{
> +	TEST_LOOKUP(arr_input, "55", 1, 1);
> +}
>   
> -	if (!TEST(setup(), "setup"))
> -		test_skip_all("hash algo initialization failed");
> +void test_oid_array__lookup_non_existent(void)
> +{
> +	TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
> +}
> +
> +void test_oid_array__lookup_duplicates(void)
> +{
> +	TEST_LOOKUP(arr_input_dup, "55", 3, 5);
> +}
>   
> -	TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
> -	TEST_ENUMERATION(arr_input_dup, res_sorted,
> -			 "ordered enumeration with duplicate suppression");
> +void test_oid_array__lookup_non_existent_dup(void)
> +{
> +	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
> +}
>   
> -	TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
> -	TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
> -	TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
> -	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
> -		    "lookup non-existent entry with duplicates");
> +void test_oid_array__lookup_almost_dup(void)
> +{
> +	const char *nearly_55;
>   
>   	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
>   			"5500000000000000000000000000000000000001" :
>   			"5500000000000000000000000000000000000000000000000000000000000001";
> -	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
> -		    "lookup with almost duplicate values");
> -	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
> -		    "lookup with single duplicate value");
>   
> -	return test_done();
> +	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
> +}
> +
> +void test_oid_array__lookup_single_dup(void)
> +{
> +	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
>   }

Re: [PATCH 1/5] t/unit-tests: implement oid helper functions in unit-tests.{c,h}

[Patrick Steinhardt]

On Thu, Feb 20, 2025 at 02:38:21PM +0000, Phillip Wood wrote:
> Hi Seyi
> 
> On 20/02/2025 08:29, Seyi Kuforiji wrote:
> > `get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
> > oid-related tests to run without errors. In the current implementation,
> > both functions are defined and declared in the
> > `t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
> > the homegrown unit tests structure.
> > 
> > Implement equivalent functions in unit-tests.{c,h}. Both these functions
> > become available for oid-related test files implemented using the clar
> > testing framework, which requires them. This will be used by subsequent
> > commits.
> 
> It is nice to see these tests being moved over to clar but I'm not sure that
> moving these functions into this file is good idea. All the unit tests need
> to link against unit-tests.o but only a subset will want access to these
> functions. Putting them in this file means that all the tests will now
> depend on code from strbuf.o and hex.o. I think we could add the new
> functions to lib-oid.c and then remove the old ones when there are not
> needed any more.

That should probably work, yeah. In that case we'd have to rename
`init_hash_algo()`, e.g. to something like `cl_setup_hash_algo()`, to
clearly show that it is part of the clar testing framework. I think
having a common prefix for such helper functions would be good anyway so
that the symbols never conflict with symbols we have in libgit.

Patrick

Re: [PATCH 1/5] t/unit-tests: implement oid helper functions in unit-tests.{c,h}

[Patrick Steinhardt]

On Thu, Feb 20, 2025 at 09:29:55AM +0100, Seyi Kuforiji wrote:
> diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
> index 85e5d6a948..ebed51212f 100644
> --- a/t/unit-tests/unit-test.h
> +++ b/t/unit-tests/unit-test.h
> @@ -8,3 +8,22 @@
>  	snprintf(desc, sizeof(desc), fmt, __VA_ARGS__); \
>  	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
>  } while (0)
> +
> +/*
> + * Convert arbitrary hex string to object_id.
> + * For example, passing "abc12" will generate
> + * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
> + * create object_id with that.
> + * WARNING: passing a string of length more than the hexsz of respective hash
> + * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
> + * environment variable.
> + */
> +void cl_parse_any_oid(const char *s, struct object_id *oid);

Nit: let's add a space between the function decarations. I'd also add a
blank line into the comments before the "For example" paragraph and
after it to make it easier to parse.

Patrick

Re: [PATCH 3/5] t/unit-tests: convert oidmap test to use clar

[phillip.wood123]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Adapt oidmap test script to clar framework by using clar assertions
> where necessary. `cl_parse_any_oid` handles the necessary checks needed
> for the test to run smoothly.

I'm not sure what the last half of this sentence means. What checks are 
performed and how does that lead to the test running smoothly?

> Introduce 'test_oidmap__initialize` handles the to set up of the global
> oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
> frees the oidmap and its entries when all tests are completed.
> 
> This streamlines the test suite, making individual tests self-contained
> and reducing redundant code.

This seems to be saying that by sharing global state we're making the 
tests self-contained - I'm not sure how that can be true. We need to 
move to sharing a single oidmap between all the tests because clar's 
setup and teardown functions don't take a context pointer. That's fine 
but I don't see how it makes the tests self-contained.

Everything up to this point looks good.

>   	while ((entry = oidmap_iter_next(&iter))) {
> -		int ret;
> -		if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {
> -			switch (ret) {
> -			case -1:
> -				break; /* error message handled by get_oid_arbitrary_hex() */
> -			case 1:
> -				test_msg("obtained entry was not given in the input\n"
> -					 "  name: %s\n   oid: %s\n",
> -					 entry->name, oid_to_hex(&entry->entry.oid));
> -				break;
> -			case 2:
> -				test_msg("duplicate entry detected\n"
> -					 "  name: %s\n   oid: %s\n",
> -					 entry->name, oid_to_hex(&entry->entry.oid));
> -				break;
> -			default:
> -				test_msg("BUG: invalid return value (%d) from key_val_contains()",
> -					 ret);
> -				break;
> -			}
> -		} else {
> -			count++;
> -		}
> +		cl_assert_equal_i(key_val_contains(entry, seen), 0);

I think wed' be better to use clar_fail_f() so that we can keep the 
helpful error messages. Using cl_assert_equal_i() isn't terrible as if 
the test fails at least we know the error code but as we already have 
the logic in place to provide better messages lets adapt it.

There is a change of behavior here as before we'd loop through the whole 
list of entries detecting all the errors. Now we quit on the first 
error. I don't think that matters but it would be good to point out the 
change in the commit message.

Best Wishes

Phillip

Re: [PATCH 4/5] t/unit-tests: convert oidtree test to use clar

[phillip.wood123]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Adapt oidtree test script to clar framework by using clar assertions
> where necessary. `cl_parse_any_oid` handles the necessary checks needed
> for the test to run smoothly.
> 
> Introduce 'test_oidtree__initialize` handles the to set up of the global
> oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
> tests are completed.
> 
> This streamlines the test suite, making individual tests self-contained
> and reducing redundant code.

My comments on the commit message for the last patch apply here as well.

> -	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
> -					     &expected), ==, 0))
> -		; /* the data is bogus and cannot be used */
> -	else if (!check(oideq(oid, &expected)))
> -		test_msg("expected: %s\n       got: %s\n     query: %s",
> -			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
> +	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
>   
> +	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
> +			 &expected);

using cl_parse_any_oid() means that it is safe to dispense with the 
check on the return value because we now do that check in cl_parse_oid().

> +	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));

This is nice - we'll get a message containing the two oid's if they 
don't match. We should use the same trick in patch 2.

As with the last patch we now bail out after the first error, we should 
mention that in the commit message.

> -	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
> -		test_msg("error: could not find some 'object_id's for query ('%s')", query);
> +	cl_assert_equal_i(hex_iter.i, hex_iter.expected_hexes.nr);

Using cl_failf() here would enable us to keep the message so that if the 
test fails we can see which query it was that failed.

Apart from that last point this is looking good

Best Wishes

Phillip

Re: [PATCH 1/5] t/unit-tests: implement oid helper functions in unit-tests.{c,h}

[phillip.wood123]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:

> +static void cl_parse_oid(const char *hex, struct object_id *oid,
> +				       const struct git_hash_algo *algop)
> +{
> +	int ret;
> +	size_t sz = strlen(hex);
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	cl_assert(sz <= algop->hexsz);
> +
> +	strbuf_add(&buf, hex, sz);
> +	strbuf_addchars(&buf, '0', algop->hexsz - sz);
> +
> +	ret = get_oid_hex_algop(buf.buf, oid, algop);
> +	cl_assert_equal_i(ret, 0);

These last two lines would be better written as

	cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);

So that if it fails the message shows which function was being called

Best Wishes

Phillip

Re: [PATCH 0/5] t/unit-tests: convert unit-tests to use clar

[phillip.wood123]

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Hello,
> 
> This small patch series transitions a couple more of our existing unit
> test files to the Clar testing framework. This change is part of our
> ongoing effort to standardize our testing framework to enhance
> maintainability.

Overall I think the conversions are correct, but we'd be better to keep 
the diagnostic messages by using cl_failf().

Best Wishes

Phillip

> Thanks
> Seyi
> 
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> 
> Seyi Kuforiji (5):
>    t/unit-tests: implement oid helper functions in unit-tests.{c,h}
>    t/unit-tests: convert oid-array test to use clar test framework
>    t/unit-tests: convert oidmap test to use clar test framework
>    t/unit-tests: convert oidtree test to use clar test framework
>    t/unit-tests: remove lib-oid.{c,h,o}
> 
>   Makefile                                      |   7 +-
>   t/meson.build                                 |   7 +-
>   t/unit-tests/lib-oid.c                        |  52 ------
>   t/unit-tests/lib-oid.h                        |  25 ---
>   t/unit-tests/{t-oid-array.c => u-oid-array.c} | 123 +++++++-------
>   t/unit-tests/{t-oidmap.c => u-oidmap.c}       | 153 ++++++------------
>   t/unit-tests/{t-oidtree.c => u-oidtree.c}     |  78 ++++-----
>   t/unit-tests/unit-test.c                      |  42 +++++
>   t/unit-tests/unit-test.h                      |  19 +++
>   9 files changed, 212 insertions(+), 294 deletions(-)
>   delete mode 100644 t/unit-tests/lib-oid.c
>   delete mode 100644 t/unit-tests/lib-oid.h
>   rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (35%)
>   rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)
>   rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (44%)
> 

Re: [PATCH 2/5] t/unit-tests: convert oid-array test to use clar

[Seyi Chamber]

On Thu, 20 Feb 2025 at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Seyi
>
> On 20/02/2025 08:29, Seyi Kuforiji wrote:
> > Adapt oid-array test script to clar framework by using clar assertions
> > where necessary. Remove descriptions from macros to reduce
> > redundancy, and move test input arrays to global scope for reuse across
> > multiple test functions. Introduce `test_oid_array__initialize()` to
> > explicitly initialize the hash algorithm.
> >
> > These changes streamline the test suite, making individual tests
> > self-contained and reducing redundant code.
>
> I think these conversion look correct but once again we're losing
> valuable debugging information because we haven't added better
> assertions to clear.
>

I understand your concern about losing debugging information, but it
is more beneficial to prioritize clarity and simplicity in unit tests.
Unit tests should be short and easy, and adding extra debugging
messages increases complexity, making them harder to maintain and
read. The assertion failures already indicate where an issue occurs,
allowing whoever is debugging to inspect the test inputs directly if
needed.

Assertion failures are rarely hit in real-world scenarios, and when
they do occur, one can manually print values or use a debugger to
investigate. This makes the additional debugging messages unnecessary
in most cases. The lack of explicit debugging output is not a
significant issue in practice. Furthermore, there are plans to
collaborate with Clar upstream to equip common assertions with the
ability to print custom messages in a formatted string where an error
occurs. This would allow our test to be simple and easy to read and
also maintain some of our custom debug messages.



> >       oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> > -     if (!check_uint(actual.nr, ==, expect.nr))
> > -             return;
> > -
> > -     for (i = 0; i < actual.nr; i++) {
> > -             if (!check(oideq(&actual.oid[i], &expect.oid[i])))
> > -                     test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
> > -                              oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
> > -                              (uintmax_t)i);
> > -     }
> > +     cl_assert_equal_i(actual.nr, expect.nr);
> > +
> > +     for (i = 0; i < actual.nr; i++)
> > +             cl_assert(oideq(&actual.oid[i], &expect.oid[i]));
>
> If this fails the poor person debugging it will have no idea why as
> there is now no indication of which two oids were being compared.
>
> > -     if (!check_int(ret, <=, upper_bound) ||
> > -         !check_int(ret, >=, lower_bound))
> > -             test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
> > +     cl_assert(ret <= upper_bound);
> > +     cl_assert(ret >= lower_bound);
>
> This is another case where we could do with better assertions in clar
>
> > -static void setup(void)
> > +void test_oid_array__initialize(void)
> >   {
> >       /* The hash algo is used by oid_array_lookup() internally */
> >       int algo = init_hash_algo();
> > -     if (check_int(algo, !=, GIT_HASH_UNKNOWN))
> > -             repo_set_hash_algo(the_repository, algo);
> > +     cl_assert(algo != GIT_HASH_UNKNOWN);
>
> init_has_algo() in unit-test.c already does this.
>

Thanks for spotting this! Will fix this in an updated patch.
> Best Wishes
>
> Phillip
>
>
> > +     repo_set_hash_algo(the_repository, algo);
> >   }
> >
> > -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> > +static const char *arr_input[] = { "88", "44", "aa", "55" };
> > +static const char *arr_input_dup[] = { "88", "44", "aa", "55",
> > +                                    "88", "44", "aa", "55",
> > +                                    "88", "44", "aa", "55" };
> > +static const char *res_sorted[] = { "44", "55", "88", "aa" };
> > +
> > +void test_oid_array__enumerate_unique(void)
> >   {
> > -     const char *arr_input[] = { "88", "44", "aa", "55" };
> > -     const char *arr_input_dup[] = { "88", "44", "aa", "55",
> > -                                     "88", "44", "aa", "55",
> > -                                     "88", "44", "aa", "55" };
> > -     const char *res_sorted[] = { "44", "55", "88", "aa" };
> > -     const char *nearly_55;
> > +     TEST_ENUMERATION(arr_input, res_sorted);
> > +}
> > +
> > +void test_oid_array__enumerate_duplicate(void)
> > +{
> > +     TEST_ENUMERATION(arr_input_dup, res_sorted);
> > +}
> > +
> > +void test_oid_array__lookup(void)
> > +{
> > +     TEST_LOOKUP(arr_input, "55", 1, 1);
> > +}
> >
> > -     if (!TEST(setup(), "setup"))
> > -             test_skip_all("hash algo initialization failed");
> > +void test_oid_array__lookup_non_existent(void)
> > +{
> > +     TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
> > +}
> > +
> > +void test_oid_array__lookup_duplicates(void)
> > +{
> > +     TEST_LOOKUP(arr_input_dup, "55", 3, 5);
> > +}
> >
> > -     TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
> > -     TEST_ENUMERATION(arr_input_dup, res_sorted,
> > -                      "ordered enumeration with duplicate suppression");
> > +void test_oid_array__lookup_non_existent_dup(void)
> > +{
> > +     TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
> > +}
> >
> > -     TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
> > -     TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
> > -     TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
> > -     TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
> > -                 "lookup non-existent entry with duplicates");
> > +void test_oid_array__lookup_almost_dup(void)
> > +{
> > +     const char *nearly_55;
> >
> >       nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
> >                       "5500000000000000000000000000000000000001" :
> >                       "5500000000000000000000000000000000000000000000000000000000000001";
> > -     TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
> > -                 "lookup with almost duplicate values");
> > -     TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
> > -                 "lookup with single duplicate value");
> >
> > -     return test_done();
> > +     TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
> > +}
> > +
> > +void test_oid_array__lookup_single_dup(void)
> > +{
> > +     TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
> >   }
>

Thanks
Seyi

Re: [PATCH 2/5] t/unit-tests: convert oid-array test to use clar

[phillip.wood123]

Hi Seyi

On 24/02/2025 09:11, Seyi Chamber wrote:
> On Thu, 20 Feb 2025 at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Seyi
>>
>> On 20/02/2025 08:29, Seyi Kuforiji wrote:
>>> Adapt oid-array test script to clar framework by using clar assertions
>>> where necessary. Remove descriptions from macros to reduce
>>> redundancy, and move test input arrays to global scope for reuse across
>>> multiple test functions. Introduce `test_oid_array__initialize()` to
>>> explicitly initialize the hash algorithm.
>>>
>>> These changes streamline the test suite, making individual tests
>>> self-contained and reducing redundant code.
>>
>> I think these conversion look correct but once again we're losing
>> valuable debugging information because we haven't added better
>> assertions to clear.
>>
> 
> I understand your concern about losing debugging information, but it
> is more beneficial to prioritize clarity and simplicity in unit tests.
> Unit tests should be short and easy, and adding extra debugging
> messages increases complexity, making them harder to maintain and
> read. The assertion failures already indicate where an issue occurs,
> allowing whoever is debugging to inspect the test inputs directly if
> needed.

If the test failure is on a CI run for a platform that the person 
debugging the failure does not have access to how are they going to do 
that? This is not a hypothetical concern as our CI runs the test suite 
on MacOs, Linux and Windows. Individual developers often only have 
access to one or to of those platforms. My experience of debugging CI 
test failures is that without decent diagnostic messages it is extremely 
difficult to figure out what went wrong if one does not have access to 
the platform where the test is failing.

> Assertion failures are rarely hit in real-world scenarios, and when
> they do occur, one can manually print values or use a debugger to
> investigate. This makes the additional debugging messages unnecessary
> in most cases.

This seems to be arguing that because we expect the tests to pass we 
don't need to worry about how difficult it is to debug them when they 
fail. I do not agree with that line of argument.

> The lack of explicit debugging output is not a
> significant issue in practice. Furthermore, there are plans to
> collaborate with Clar upstream to equip common assertions with the
> ability to print custom messages in a formatted string where an error
> occurs. This would allow our test to be simple and easy to read and
> also maintain some of our custom debug messages.

Have you got any more details about this please? We already have 
cl_failf() in our codebase.

Best Wishes

Phillip

> 
> 
>>>        oid_array_for_each_unique(&input, add_to_oid_array, &actual);
>>> -     if (!check_uint(actual.nr, ==, expect.nr))
>>> -             return;
>>> -
>>> -     for (i = 0; i < actual.nr; i++) {
>>> -             if (!check(oideq(&actual.oid[i], &expect.oid[i])))
>>> -                     test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
>>> -                              oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
>>> -                              (uintmax_t)i);
>>> -     }
>>> +     cl_assert_equal_i(actual.nr, expect.nr);
>>> +
>>> +     for (i = 0; i < actual.nr; i++)
>>> +             cl_assert(oideq(&actual.oid[i], &expect.oid[i]));
>>
>> If this fails the poor person debugging it will have no idea why as
>> there is now no indication of which two oids were being compared.
>>
>>> -     if (!check_int(ret, <=, upper_bound) ||
>>> -         !check_int(ret, >=, lower_bound))
>>> -             test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
>>> +     cl_assert(ret <= upper_bound);
>>> +     cl_assert(ret >= lower_bound);
>>
>> This is another case where we could do with better assertions in clar
>>
>>> -static void setup(void)
>>> +void test_oid_array__initialize(void)
>>>    {
>>>        /* The hash algo is used by oid_array_lookup() internally */
>>>        int algo = init_hash_algo();
>>> -     if (check_int(algo, !=, GIT_HASH_UNKNOWN))
>>> -             repo_set_hash_algo(the_repository, algo);
>>> +     cl_assert(algo != GIT_HASH_UNKNOWN);
>>
>> init_has_algo() in unit-test.c already does this.
>>
> 
> Thanks for spotting this! Will fix this in an updated patch.
>> Best Wishes
>>
>> Phillip
>>
>>
>>> +     repo_set_hash_algo(the_repository, algo);
>>>    }
>>>
>>> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
>>> +static const char *arr_input[] = { "88", "44", "aa", "55" };
>>> +static const char *arr_input_dup[] = { "88", "44", "aa", "55",
>>> +                                    "88", "44", "aa", "55",
>>> +                                    "88", "44", "aa", "55" };
>>> +static const char *res_sorted[] = { "44", "55", "88", "aa" };
>>> +
>>> +void test_oid_array__enumerate_unique(void)
>>>    {
>>> -     const char *arr_input[] = { "88", "44", "aa", "55" };
>>> -     const char *arr_input_dup[] = { "88", "44", "aa", "55",
>>> -                                     "88", "44", "aa", "55",
>>> -                                     "88", "44", "aa", "55" };
>>> -     const char *res_sorted[] = { "44", "55", "88", "aa" };
>>> -     const char *nearly_55;
>>> +     TEST_ENUMERATION(arr_input, res_sorted);
>>> +}
>>> +
>>> +void test_oid_array__enumerate_duplicate(void)
>>> +{
>>> +     TEST_ENUMERATION(arr_input_dup, res_sorted);
>>> +}
>>> +
>>> +void test_oid_array__lookup(void)
>>> +{
>>> +     TEST_LOOKUP(arr_input, "55", 1, 1);
>>> +}
>>>
>>> -     if (!TEST(setup(), "setup"))
>>> -             test_skip_all("hash algo initialization failed");
>>> +void test_oid_array__lookup_non_existent(void)
>>> +{
>>> +     TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
>>> +}
>>> +
>>> +void test_oid_array__lookup_duplicates(void)
>>> +{
>>> +     TEST_LOOKUP(arr_input_dup, "55", 3, 5);
>>> +}
>>>
>>> -     TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
>>> -     TEST_ENUMERATION(arr_input_dup, res_sorted,
>>> -                      "ordered enumeration with duplicate suppression");
>>> +void test_oid_array__lookup_non_existent_dup(void)
>>> +{
>>> +     TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
>>> +}
>>>
>>> -     TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
>>> -     TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
>>> -     TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
>>> -     TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
>>> -                 "lookup non-existent entry with duplicates");
>>> +void test_oid_array__lookup_almost_dup(void)
>>> +{
>>> +     const char *nearly_55;
>>>
>>>        nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
>>>                        "5500000000000000000000000000000000000001" :
>>>                        "5500000000000000000000000000000000000000000000000000000000000001";
>>> -     TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
>>> -                 "lookup with almost duplicate values");
>>> -     TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
>>> -                 "lookup with single duplicate value");
>>>
>>> -     return test_done();
>>> +     TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
>>> +}
>>> +
>>> +void test_oid_array__lookup_single_dup(void)
>>> +{
>>> +     TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
>>>    }
>>
> 
> Thanks
> Seyi

Re: [PATCH 3/5] t/unit-tests: convert oidmap test to use clar

[Seyi Chamber]

On Fri, 21 Feb 2025 at 11:04, <phillip.wood123@gmail.com> wrote:
>
> Hi Seyi
>
> On 20/02/2025 08:29, Seyi Kuforiji wrote:
> > Adapt oidmap test script to clar framework by using clar assertions
> > where necessary. `cl_parse_any_oid` handles the necessary checks needed
> > for the test to run smoothly.
>
> I'm not sure what the last half of this sentence means. What checks are
> performed and how does that lead to the test running smoothly?
>

`cl_parse_any_oid()` ensures the hash algorithm is set before parsing.
This prevents issues from an uninitialized or invalid hash algorithm.
Without these checks, the test could behave unpredictably. I’ll update
the commit message to make this clearer.

> > Introduce 'test_oidmap__initialize` handles the to set up of the global
> > oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
> > frees the oidmap and its entries when all tests are completed.
> >
> > This streamlines the test suite, making individual tests self-contained
> > and reducing redundant code.
>
> This seems to be saying that by sharing global state we're making the
> tests self-contained - I'm not sure how that can be true. We need to
> move to sharing a single oidmap between all the tests because clar's
> setup and teardown functions don't take a context pointer. That's fine
> but I don't see how it makes the tests self-contained.
>

Right! Thank you. I'll adjust the commit message accordingly.

> Everything up to this point looks good.
>
> >       while ((entry = oidmap_iter_next(&iter))) {
> > -             int ret;
> > -             if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {
> > -                     switch (ret) {
> > -                     case -1:
> > -                             break; /* error message handled by get_oid_arbitrary_hex() */
> > -                     case 1:
> > -                             test_msg("obtained entry was not given in the input\n"
> > -                                      "  name: %s\n   oid: %s\n",
> > -                                      entry->name, oid_to_hex(&entry->entry.oid));
> > -                             break;
> > -                     case 2:
> > -                             test_msg("duplicate entry detected\n"
> > -                                      "  name: %s\n   oid: %s\n",
> > -                                      entry->name, oid_to_hex(&entry->entry.oid));
> > -                             break;
> > -                     default:
> > -                             test_msg("BUG: invalid return value (%d) from key_val_contains()",
> > -                                      ret);
> > -                             break;
> > -                     }
> > -             } else {
> > -                     count++;
> > -             }
> > +             cl_assert_equal_i(key_val_contains(entry, seen), 0);
>
> I think wed' be better to use clar_fail_f() so that we can keep the
> helpful error messages. Using cl_assert_equal_i() isn't terrible as if
> the test fails at least we know the error code but as we already have
> the logic in place to provide better messages lets adapt it.
>
> There is a change of behavior here as before we'd loop through the whole
> list of entries detecting all the errors. Now we quit on the first
> error. I don't think that matters but it would be good to point out the
> change in the commit message.
>
> Best Wishes
>
> Phillip
Noted! thank you.
Seyi

[PATCH v2 0/4] t/unit-tests: convert unit-tests to use clar

[Seyi Kuforiji]

Hello,

This small patch series transitions a couple more of our existing unit
test files to the Clar testing framework. This change is part of our
ongoing effort to standardize our testing framework to enhance
maintainability.

Changes in v2:
 - fixes to the commit messages and how they read
 - some code refactoring based on review

Thanks
Seyi

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Philip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>

Seyi Kuforiji (4):
  t/unit-tests: implement clar specific oid helper functions
  t/unit-tests: convert oid-array test to use clar test framework
  t/unit-tests: convert oidmap test to use clar test framework
  t/unit-tests: convert oidtree test to use clar test framework

 Makefile                                      |   8 +-
 t/meson.build                                 |   8 +-
 t/unit-tests/lib-oid.c                        |  31 ++--
 t/unit-tests/lib-oid.h                        |   9 +-
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 125 +++++++-------
 t/unit-tests/{t-oidmap.c => u-oidmap.c}       | 153 +++++++-----------
 t/unit-tests/{t-oidtree.c => u-oidtree.c}     |  79 ++++-----
 t/unit-tests/unit-test.c                      |   2 +
 8 files changed, 177 insertions(+), 238 deletions(-)
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (34%)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (45%)

Range-diff against v1:
1:  19192c6c89 < -:  ---------- t/unit-tests: implement oid helper functions in unit-tests.{c,h}
5:  e81ec73f27 ! 1:  7f14d0d574 t/unit-tests: remove lib-oid.{c,h,o}
    @@ Metadata
     Author: Seyi Kuforiji <kuforiji98@gmail.com>
     
      ## Commit message ##
    -    t/unit-tests: remove lib-oid.{c,h,o}
    +    t/unit-tests: implement clar specific oid helper functions
     
    -    The `lib-oid.c`, `lib-oid.h`, and `lib-oid.o files` are no longer needed
    -    since their equivalent functions have been implemented in unit-test.c
    -    and unit-test.h. This removes redundant code and ensures all unit
    -    test-related functionality is consolidated in a single location.
    +    `get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
    +    oid-related tests to run without errors. In the current implementation,
    +    both functions are defined and declared in the
    +    `t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
    +    the homegrown unit tests structure.
     
    -    Drop references to lib-oid from our `Makefile`, and `meson.build` files
    -    to prevent build errors due to missing files.
    +    Adapt functions in lib-oid.{c,h} to use clar. Both these functions
    +    become available for oid-related test files implemented using the clar
    +    testing framework, which requires them. This will be used by subsequent
    +    commits.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
     
      ## Makefile ##
    +@@ Makefile: CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
    + CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
    + 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-oid-array
    + UNIT_TEST_PROGRAMS += t-oidmap
     @@ Makefile: UNIT_TEST_PROGRAMS += t-trailer
      UNIT_TEST_PROGRAMS += t-urlmatch-normalization
      UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
    @@ Makefile: UNIT_TEST_PROGRAMS += t-trailer
      # xdiff and reftable libs may in turn depend on what is in libgit.a
     
      ## t/meson.build ##
    +@@ t/meson.build: clar_test_suites = [
    + clar_sources = [
    +   'unit-tests/clar/clar.c',
    +   'unit-tests/unit-test.c',
    ++  'unit-tests/lib-oid.c'
    + ]
    + 
    + clar_decls_h = custom_target(
     @@ t/meson.build: foreach unit_test_program : unit_test_programs
        unit_test = executable(unit_test_name,
          sources: [
    @@ t/meson.build: foreach unit_test_program : unit_test_programs
            unit_test_program,
          ],
     
    - ## t/unit-tests/lib-oid.c (deleted) ##
    + ## t/unit-tests/lib-oid.c ##
     @@
     -#include "test-lib.h"
    --#include "lib-oid.h"
    --#include "strbuf.h"
    --#include "hex.h"
    --
    ++#include "unit-test.h"
    + #include "lib-oid.h"
    + #include "strbuf.h"
    + #include "hex.h"
    + 
     -int init_hash_algo(void)
    --{
    --	static int algo = -1;
    --
    --	if (algo < 0) {
    --		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
    --		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
    --
    ++int cl_setup_hash_algo(void)
    + {
    + 	static int algo = -1;
    + 
    +@@ t/unit-tests/lib-oid.c: int init_hash_algo(void)
    + 		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
    + 		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
    + 
     -		if (!check(algo != GIT_HASH_UNKNOWN))
     -			test_msg("BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')",
     -				 algo_name);
    --	}
    --	return algo;
    --}
    --
    ++		cl_assert(algo != GIT_HASH_UNKNOWN);
    + 	}
    + 	return algo;
    + }
    + 
     -static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
    --				       const struct git_hash_algo *algop)
    --{
    --	int ret;
    --	size_t sz = strlen(hex);
    --	struct strbuf buf = STRBUF_INIT;
    --
    ++static void cl_parse_oid(const char *hex, struct object_id *oid,
    + 				       const struct git_hash_algo *algop)
    + {
    + 	int ret;
    + 	size_t sz = strlen(hex);
    + 	struct strbuf buf = STRBUF_INIT;
    + 
     -	if (!check(sz <= algop->hexsz)) {
     -		test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
     -			 hex, (unsigned long)algop->hexsz);
     -		return -1;
     -	}
    --
    --	strbuf_add(&buf, hex, sz);
    --	strbuf_addchars(&buf, '0', algop->hexsz - sz);
    --
    ++	cl_assert(sz <= algop->hexsz);
    + 
    + 	strbuf_add(&buf, hex, sz);
    + 	strbuf_addchars(&buf, '0', algop->hexsz - sz);
    + 
     -	ret = get_oid_hex_algop(buf.buf, oid, algop);
     -	if (!check_int(ret, ==, 0))
     -		test_msg("BUG: invalid hex input (%s) provided", hex);
    --
    --	strbuf_release(&buf);
    ++	cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
    + 
    + 	strbuf_release(&buf);
     -	return ret;
    --}
    --
    + }
    + 
     -int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)
    --{
    ++
    ++void cl_parse_any_oid(const char *hex, struct object_id *oid)
    + {
     -	int hash_algo = init_hash_algo();
    --
    ++	int hash_algo = cl_setup_hash_algo();
    + 
     -	if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))
     -		return -1;
     -	return get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);
    --}
    ++	cl_assert(hash_algo != GIT_HASH_UNKNOWN);
    ++	cl_parse_oid(hex, oid, &hash_algos[hash_algo]);
    + }
     
    - ## t/unit-tests/lib-oid.h (deleted) ##
    + ## t/unit-tests/lib-oid.h ##
    +@@
    + 
    + /*
    +  * Convert arbitrary hex string to object_id.
    ++ *
    +  * For example, passing "abc12" will generate
    +  * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
    +  * create object_id with that.
     @@
    --#ifndef LIB_OID_H
    --#define LIB_OID_H
    --
    --#include "hash.h"
    --
    --/*
    -- * Convert arbitrary hex string to object_id.
    -- * For example, passing "abc12" will generate
    -- * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
    -- * create object_id with that.
    -- * WARNING: passing a string of length more than the hexsz of respective hash
    -- * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
    -- * environment variable.
    -- */
    +  * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
    +  * environment variable.
    +  */
     -int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
    --/*
    -- * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
    -- * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
    -- * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
    ++
    ++void cl_parse_any_oid (const char *s, struct object_id *oid);
    + /*
    +  * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
    +  * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
    +  * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
     - * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the
    -- * GIT_TEST_DEFAULT_HASH's value is valid or not.
    -- */
    ++ * cl_assert(algo != GIT_HASH_UNKNOWN) before returning to verify if the
    +  * GIT_TEST_DEFAULT_HASH's value is valid or not.
    +  */
     -int init_hash_algo(void);
    --
    --#endif /* LIB_OID_H */
    ++
    ++int cl_setup_hash_algo(void);
    + 
    + #endif /* LIB_OID_H */
    +
    + ## t/unit-tests/unit-test.c ##
    +@@
    + #include "unit-test.h"
    ++#include "hex.h"
    + #include "parse-options.h"
    ++#include "strbuf.h"
    + #include "string-list.h"
    + #include "strvec.h"
    + 
2:  8a99bbdc31 ! 2:  430f5c5007 t/unit-tests: convert oid-array test to use clar test framework
    @@ Makefile: CLAR_TEST_SUITES += u-example-decorate
      CLAR_TEST_SUITES += u-prio-queue
      CLAR_TEST_SUITES += u-reftable-tree
      CLAR_TEST_SUITES += u-strbuf
    -@@ Makefile: CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
    - CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
    +@@ Makefile: 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-oid-array
      UNIT_TEST_PROGRAMS += t-oidmap
    @@ t/unit-tests/u-oid-array.c (new)
     +#define USE_THE_REPOSITORY_VARIABLE
     +
     +#include "unit-test.h"
    ++#include "lib-oid.h"
     +#include "oid-array.h"
     +#include "hex.h"
     +
    @@ t/unit-tests/u-oid-array.c (new)
     +void test_oid_array__initialize(void)
     +{
     +	/* The hash algo is used by oid_array_lookup() internally */
    -+	int algo = init_hash_algo();
    -+	cl_assert(algo != GIT_HASH_UNKNOWN);
    ++	int algo = cl_setup_hash_algo();
     +	repo_set_hash_algo(the_repository, algo);
     +}
     +
    @@ t/unit-tests/u-oid-array.c (new)
     +{
     +	const char *nearly_55;
     +
    -+	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
    ++	nearly_55 = cl_setup_hash_algo() == GIT_HASH_SHA1 ?
     +			"5500000000000000000000000000000000000001" :
     +			"5500000000000000000000000000000000000000000000000000000000000001";
     +
3:  c19545e2bc ! 3:  319cea1265 t/unit-tests: convert oidmap test to use clar test framework
    @@ Commit message
         t/unit-tests: convert oidmap test to use clar test framework
     
         Adapt oidmap test script to clar framework by using clar assertions
    -    where necessary. `cl_parse_any_oid` handles the necessary checks needed
    -    for the test to run smoothly.
    +    where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
    +    before parsing. This prevents issues from an uninitialized or invalid
    +    hash algorithm.
     
         Introduce 'test_oidmap__initialize` handles the to set up of the global
         oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
         frees the oidmap and its entries when all tests are completed.
     
    -    This streamlines the test suite, making individual tests self-contained
    -    and reducing redundant code.
    +    The test loops through all entries to detect multiple errors. With this
    +    change, it stops at the first error encountered, making it easier to
    +    address it.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
    @@ Makefile: CLAR_TEST_SUITES += u-hash
      CLAR_TEST_SUITES += u-prio-queue
      CLAR_TEST_SUITES += u-reftable-tree
      CLAR_TEST_SUITES += u-strbuf
    -@@ Makefile: CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
    - CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
    +@@ Makefile: 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-oidmap
      UNIT_TEST_PROGRAMS += t-oidtree
    @@ t/unit-tests/t-oidmap.c (deleted)
      ## t/unit-tests/u-oidmap.c (new) ##
     @@
     +#include "unit-test.h"
    ++#include "lib-oid.h"
     +#include "oidmap.h"
     +#include "hash.h"
     +#include "hex.h"
    @@ t/unit-tests/u-oidmap.c (new)
     +
     +	oidmap_iter_init(&map, &iter);
     +	while ((entry = oidmap_iter_next(&iter))) {
    -+		cl_assert_equal_i(key_val_contains(entry, seen), 0);
    ++		if (key_val_contains(entry, seen) != 0) {
    ++			cl_failf("Unexpected entry: name = %s, oid = %s",
    ++				 entry->name, oid_to_hex(&entry->entry.oid));
    ++		}
     +		count++;
     +	}
     +	cl_assert_equal_i(count, ARRAY_SIZE(key_val));
4:  733b53cd05 ! 4:  ea63a5c9f1 t/unit-tests: convert oidtree test to use clar test framework
    @@ Commit message
         t/unit-tests: convert oidtree test to use clar test framework
     
         Adapt oidtree test script to clar framework by using clar assertions
    -    where necessary. `cl_parse_any_oid` handles the necessary checks needed
    -    for the test to run smoothly.
    +    where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
    +    before parsing. This prevents issues from an uninitialized or invalid
    +    hash algorithm.
     
         Introduce 'test_oidtree__initialize` handles the to set up of the global
         oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
         tests are completed.
     
    -    This streamlines the test suite, making individual tests self-contained
    -    and reducing redundant code.
    +    With this change, `check_each` stops at the first error encountered,
    +    making it easier to address it.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
    @@ Makefile: CLAR_TEST_SUITES += u-hashmap
      CLAR_TEST_SUITES += u-prio-queue
      CLAR_TEST_SUITES += u-reftable-tree
      CLAR_TEST_SUITES += u-strbuf
    -@@ Makefile: CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
    - CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
    +@@ Makefile: 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-oidtree
      UNIT_TEST_PROGRAMS += t-reftable-basics
    @@ t/unit-tests/t-oidtree.c (deleted)
      ## t/unit-tests/u-oidtree.c (new) ##
     @@
     +#include "unit-test.h"
    ++#include "lib-oid.h"
     +#include "oidtree.h"
     +#include "hash.h"
     +#include "hex.h"
    @@ t/unit-tests/u-oidtree.c (new)
     +	cl_parse_any_oid(query, &oid);
     +	oidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);
     +
    -+	cl_assert_equal_i(hex_iter.i, hex_iter.expected_hexes.nr);
    ++	if (hex_iter.i != hex_iter.expected_hexes.nr)
    ++		cl_failf("error: could not find some 'object_id's for query ('%s')", query);
    ++
     +	strvec_clear(&hex_iter.expected_hexes);
     +}
     +
-- 
2.47.0.86.g15030f9556

[PATCH v2 1/4] t/unit-tests: implement clar specific oid helper functions

[Seyi Kuforiji]

`get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
oid-related tests to run without errors. In the current implementation,
both functions are defined and declared in the
`t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
the homegrown unit tests structure.

Adapt functions in lib-oid.{c,h} to use clar. Both these functions
become available for oid-related test files implemented using the clar
testing framework, which requires them. This will be used by subsequent
commits.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                 |  2 +-
 t/meson.build            |  2 +-
 t/unit-tests/lib-oid.c   | 31 +++++++++++--------------------
 t/unit-tests/lib-oid.h   |  9 ++++++---
 t/unit-tests/unit-test.c |  2 ++
 5 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/Makefile b/Makefile
index bcf5ed3f85..81799488f0 100644
--- a/Makefile
+++ b/Makefile
@@ -1365,6 +1365,7 @@ CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 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-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
@@ -1381,7 +1382,6 @@ UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
-UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
diff --git a/t/meson.build b/t/meson.build
index 780939d49f..862cf1cfd4 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -14,6 +14,7 @@ clar_test_suites = [
 clar_sources = [
   'unit-tests/clar/clar.c',
   'unit-tests/unit-test.c',
+  'unit-tests/lib-oid.c'
 ]
 
 clar_decls_h = custom_target(
@@ -68,7 +69,6 @@ foreach unit_test_program : unit_test_programs
   unit_test = executable(unit_test_name,
     sources: [
       'unit-tests/test-lib.c',
-      'unit-tests/lib-oid.c',
       'unit-tests/lib-reftable.c',
       unit_test_program,
     ],
diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c
index 8f0ccac532..8bf09ea1cb 100644
--- a/t/unit-tests/lib-oid.c
+++ b/t/unit-tests/lib-oid.c
@@ -1,9 +1,9 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "strbuf.h"
 #include "hex.h"
 
-int init_hash_algo(void)
+int cl_setup_hash_algo(void)
 {
 	static int algo = -1;
 
@@ -11,42 +11,33 @@ int init_hash_algo(void)
 		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
 		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
 
-		if (!check(algo != GIT_HASH_UNKNOWN))
-			test_msg("BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')",
-				 algo_name);
+		cl_assert(algo != GIT_HASH_UNKNOWN);
 	}
 	return algo;
 }
 
-static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
+static void cl_parse_oid(const char *hex, struct object_id *oid,
 				       const struct git_hash_algo *algop)
 {
 	int ret;
 	size_t sz = strlen(hex);
 	struct strbuf buf = STRBUF_INIT;
 
-	if (!check(sz <= algop->hexsz)) {
-		test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
-			 hex, (unsigned long)algop->hexsz);
-		return -1;
-	}
+	cl_assert(sz <= algop->hexsz);
 
 	strbuf_add(&buf, hex, sz);
 	strbuf_addchars(&buf, '0', algop->hexsz - sz);
 
-	ret = get_oid_hex_algop(buf.buf, oid, algop);
-	if (!check_int(ret, ==, 0))
-		test_msg("BUG: invalid hex input (%s) provided", hex);
+	cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
 
 	strbuf_release(&buf);
-	return ret;
 }
 
-int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)
+
+void cl_parse_any_oid(const char *hex, struct object_id *oid)
 {
-	int hash_algo = init_hash_algo();
+	int hash_algo = cl_setup_hash_algo();
 
-	if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))
-		return -1;
-	return get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);
+	cl_assert(hash_algo != GIT_HASH_UNKNOWN);
+	cl_parse_oid(hex, oid, &hash_algos[hash_algo]);
 }
diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
index 4e77c04bd2..4031775104 100644
--- a/t/unit-tests/lib-oid.h
+++ b/t/unit-tests/lib-oid.h
@@ -5,6 +5,7 @@
 
 /*
  * Convert arbitrary hex string to object_id.
+ *
  * For example, passing "abc12" will generate
  * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
  * create object_id with that.
@@ -12,14 +13,16 @@
  * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
  * environment variable.
  */
-int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
+
+void cl_parse_any_oid (const char *s, struct object_id *oid);
 /*
  * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
  * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
  * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
- * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the
+ * cl_assert(algo != GIT_HASH_UNKNOWN) before returning to verify if the
  * GIT_TEST_DEFAULT_HASH's value is valid or not.
  */
-int init_hash_algo(void);
+
+int cl_setup_hash_algo(void);
 
 #endif /* LIB_OID_H */
diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
index fa8818842a..5af645048a 100644
--- a/t/unit-tests/unit-test.c
+++ b/t/unit-tests/unit-test.c
@@ -1,5 +1,7 @@
 #include "unit-test.h"
+#include "hex.h"
 #include "parse-options.h"
+#include "strbuf.h"
 #include "string-list.h"
 #include "strvec.h"
 
-- 
2.47.0.86.g15030f9556

[PATCH v2 2/4] t/unit-tests: convert oid-array test to use clar test framework

[Seyi Kuforiji]

Adapt oid-array test script to clar framework by using clar assertions
where necessary. Remove descriptions from macros to reduce
redundancy, and move test input arrays to global scope for reuse across
multiple test functions. Introduce `test_oid_array__initialize()` to
explicitly initialize the hash algorithm.

These changes streamline the test suite, making individual tests
self-contained and reducing redundant code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                      |   2 +-
 t/meson.build                                 |   2 +-
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 125 +++++++++---------
 3 files changed, 66 insertions(+), 63 deletions(-)
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (34%)

diff --git a/Makefile b/Makefile
index 81799488f0..c9b0320fcb 100644
--- a/Makefile
+++ b/Makefile
@@ -1356,6 +1356,7 @@ CLAR_TEST_SUITES += u-example-decorate
 CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
+CLAR_TEST_SUITES += u-oid-array
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1367,7 +1368,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-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
diff --git a/t/meson.build b/t/meson.build
index 862cf1cfd4..1b17a53acc 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -4,6 +4,7 @@ clar_test_suites = [
   'unit-tests/u-hash.c',
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
+  'unit-tests/u-oid-array.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -49,7 +50,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/u-oid-array.c
similarity index 34%
rename from t/unit-tests/t-oid-array.c
rename to t/unit-tests/u-oid-array.c
index 45b59a2a51..e48a433f21 100644
--- a/t/unit-tests/t-oid-array.c
+++ b/t/unit-tests/u-oid-array.c
@@ -1,22 +1,19 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oid-array.h"
 #include "hex.h"
 
-static int fill_array(struct oid_array *array, const char *hexes[], size_t n)
+static void fill_array(struct oid_array *array, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
 
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oid_array_append(array, &oid);
 	}
-	if (!check_uint(array->nr, ==, n))
-		return -1;
-	return 0;
+	cl_assert_equal_i(array->nr, n);
 }
 
 static int add_to_oid_array(const struct object_id *oid, void *data)
@@ -34,30 +31,22 @@ static void t_enumeration(const char **input_args, size_t input_sz,
 			 actual = OID_ARRAY_INIT;
 	size_t i;
 
-	if (fill_array(&input, input_args, input_sz))
-		return;
-	if (fill_array(&expect, expect_args, expect_sz))
-		return;
+	fill_array(&input, input_args, input_sz);
+	fill_array(&expect, expect_args, expect_sz);
 
 	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
-	if (!check_uint(actual.nr, ==, expect.nr))
-		return;
-
-	for (i = 0; i < actual.nr; i++) {
-		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
-			test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
-				 oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
-				 (uintmax_t)i);
-	}
+	cl_assert_equal_i(actual.nr, expect.nr);
+
+	for (i = 0; i < actual.nr; i++)
+		cl_assert(oideq(&actual.oid[i], &expect.oid[i]));
 
 	oid_array_clear(&actual);
 	oid_array_clear(&input);
 	oid_array_clear(&expect);
 }
 
-#define TEST_ENUMERATION(input, expect, desc)                                     \
-	TEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \
-			   desc " works")
+#define TEST_ENUMERATION(input, expect)                                     \
+	t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect));
 
 static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 		     int lower_bound, int upper_bound)
@@ -66,61 +55,75 @@ static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 	struct object_id oid_query;
 	int ret;
 
-	if (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))
-		return;
-	if (fill_array(&array, input_hexes, n))
-		return;
+	cl_parse_any_oid(query_hex, &oid_query);
+	fill_array(&array, input_hexes, n);
 	ret = oid_array_lookup(&array, &oid_query);
 
-	if (!check_int(ret, <=, upper_bound) ||
-	    !check_int(ret, >=, lower_bound))
-		test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
+	cl_assert(ret <= upper_bound);
+	cl_assert(ret >= lower_bound);
 
 	oid_array_clear(&array);
 }
 
-#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \
-	TEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
-		      lower_bound, upper_bound),                        \
-	     desc " works")
+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound) \
+	t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
+		      lower_bound, upper_bound);
 
-static void setup(void)
+void test_oid_array__initialize(void)
 {
 	/* The hash algo is used by oid_array_lookup() internally */
-	int algo = init_hash_algo();
-	if (check_int(algo, !=, GIT_HASH_UNKNOWN))
-		repo_set_hash_algo(the_repository, algo);
+	int algo = cl_setup_hash_algo();
+	repo_set_hash_algo(the_repository, algo);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+static const char *arr_input[] = { "88", "44", "aa", "55" };
+static const char *arr_input_dup[] = { "88", "44", "aa", "55",
+				       "88", "44", "aa", "55",
+				       "88", "44", "aa", "55" };
+static const char *res_sorted[] = { "44", "55", "88", "aa" };
+
+void test_oid_array__enumerate_unique(void)
 {
-	const char *arr_input[] = { "88", "44", "aa", "55" };
-	const char *arr_input_dup[] = { "88", "44", "aa", "55",
-					"88", "44", "aa", "55",
-					"88", "44", "aa", "55" };
-	const char *res_sorted[] = { "44", "55", "88", "aa" };
-	const char *nearly_55;
+	TEST_ENUMERATION(arr_input, res_sorted);
+}
+
+void test_oid_array__enumerate_duplicate(void)
+{
+	TEST_ENUMERATION(arr_input_dup, res_sorted);
+}
+
+void test_oid_array__lookup(void)
+{
+	TEST_LOOKUP(arr_input, "55", 1, 1);
+}
 
-	if (!TEST(setup(), "setup"))
-		test_skip_all("hash algo initialization failed");
+void test_oid_array__lookup_non_existent(void)
+{
+	TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
+}
+
+void test_oid_array__lookup_duplicates(void)
+{
+	TEST_LOOKUP(arr_input_dup, "55", 3, 5);
+}
 
-	TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
-	TEST_ENUMERATION(arr_input_dup, res_sorted,
-			 "ordered enumeration with duplicate suppression");
+void test_oid_array__lookup_non_existent_dup(void)
+{
+	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
+}
 
-	TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
-	TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
-	TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
-	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
-		    "lookup non-existent entry with duplicates");
+void test_oid_array__lookup_almost_dup(void)
+{
+	const char *nearly_55;
 
-	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
+	nearly_55 = cl_setup_hash_algo() == GIT_HASH_SHA1 ?
 			"5500000000000000000000000000000000000001" :
 			"5500000000000000000000000000000000000000000000000000000000000001";
-	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
-		    "lookup with almost duplicate values");
-	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
-		    "lookup with single duplicate value");
 
-	return test_done();
+	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
+}
+
+void test_oid_array__lookup_single_dup(void)
+{
+	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
 }
-- 
2.47.0.86.g15030f9556

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

[Seyi Kuforiji]

Adapt oidmap test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
before parsing. This prevents issues from an uninitialized or invalid
hash algorithm.

Introduce 'test_oidmap__initialize` handles the to set up of the global
oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
frees the oidmap and its entries when all tests are completed.

The test loops through all entries to detect multiple errors. With this
change, it stops at the first error encountered, making it easier to
address it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                |   2 +-
 t/meson.build                           |   2 +-
 t/unit-tests/{t-oidmap.c => u-oidmap.c} | 153 +++++++++---------------
 3 files changed, 56 insertions(+), 101 deletions(-)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)

diff --git a/Makefile b/Makefile
index c9b0320fcb..e4e85e6007 100644
--- a/Makefile
+++ b/Makefile
@@ -1357,6 +1357,7 @@ CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-oid-array
+CLAR_TEST_SUITES += u-oidmap
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1368,7 +1369,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-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
diff --git a/t/meson.build b/t/meson.build
index 1b17a53acc..d5b83cdb72 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -5,6 +5,7 @@ clar_test_suites = [
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
+  'unit-tests/u-oidmap.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -50,7 +51,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/u-oidmap.c
similarity index 32%
rename from t/unit-tests/t-oidmap.c
rename to t/unit-tests/u-oidmap.c
index b22e52d08b..dc805b7e3c 100644
--- a/t/unit-tests/t-oidmap.c
+++ b/t/unit-tests/u-oidmap.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oidmap.h"
 #include "hash.h"
@@ -18,102 +18,85 @@ static const char *const key_val[][2] = { { "11", "one" },
 					  { "22", "two" },
 					  { "33", "three" } };
 
-static void setup(void (*f)(struct oidmap *map))
+static struct oidmap map;
+
+void test_oidmap__initialize(void)
 {
-	struct oidmap map = OIDMAP_INIT;
-	int ret = 0;
+	oidmap_init(&map, 0);
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++){
 		struct test_entry *entry;
 
 		FLEX_ALLOC_STR(entry, name, key_val[i][1]);
-		if ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) {
-			free(entry);
-			break;
-		}
-		entry = oidmap_put(&map, entry);
-		if (!check(entry == NULL))
-			free(entry);
+		cl_parse_any_oid(key_val[i][0], &entry->entry.oid);
+		cl_assert(oidmap_put(&map, entry) == NULL);
 	}
+}
 
-	if (!ret)
-		f(&map);
+void test_oidmap__cleanup(void)
+{
 	oidmap_free(&map, 1);
 }
 
-static void t_replace(struct oidmap *map)
+void test_oidmap__replace(void)
 {
 	struct test_entry *entry, *prev;
 
 	FLEX_ALLOC_STR(entry, name, "un");
-	if (get_oid_arbitrary_hex("11", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "one");
+	cl_parse_any_oid("11", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "one");
 	free(prev);
 
 	FLEX_ALLOC_STR(entry, name, "deux");
-	if (get_oid_arbitrary_hex("22", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "two");
+	cl_parse_any_oid("22", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "two");
 	free(prev);
 }
 
-static void t_get(struct oidmap *map)
+void test_oidmap__get(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_get(map, &oid) == NULL);
-
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_get(&map, &oid) == NULL);
+
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
 }
 
-static void t_remove(struct oidmap *map)
+void test_oidmap__remove(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_remove(map, &oid) == NULL);
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_remove(&map, &oid) == NULL);
 }
 
 static int key_val_contains(struct test_entry *entry, char seen[])
@@ -121,8 +104,7 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		struct object_id oid;
 
-		if (get_oid_arbitrary_hex(key_val[i][0], &oid))
-			return -1;
+		cl_parse_any_oid(key_val[i][0], &oid);
 
 		if (oideq(&entry->entry.oid, &oid)) {
 			if (seen[i])
@@ -134,48 +116,21 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	return 1;
 }
 
-static void t_iterate(struct oidmap *map)
+void test_oidmap__iterate(void)
 {
 	struct oidmap_iter iter;
 	struct test_entry *entry;
 	char seen[ARRAY_SIZE(key_val)] = { 0 };
 	int count = 0;
 
-	oidmap_iter_init(map, &iter);
+	oidmap_iter_init(&map, &iter);
 	while ((entry = oidmap_iter_next(&iter))) {
-		int ret;
-		if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {
-			switch (ret) {
-			case -1:
-				break; /* error message handled by get_oid_arbitrary_hex() */
-			case 1:
-				test_msg("obtained entry was not given in the input\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			case 2:
-				test_msg("duplicate entry detected\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			default:
-				test_msg("BUG: invalid return value (%d) from key_val_contains()",
-					 ret);
-				break;
-			}
-		} else {
-			count++;
+		if (key_val_contains(entry, seen) != 0) {
+			cl_failf("Unexpected entry: name = %s, oid = %s",
+				 entry->name, oid_to_hex(&entry->entry.oid));
 		}
+		count++;
 	}
-	check_int(count, ==, ARRAY_SIZE(key_val));
-	check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));
-}
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST(setup(t_replace), "replace works");
-	TEST(setup(t_get), "get works");
-	TEST(setup(t_remove), "remove works");
-	TEST(setup(t_iterate), "iterate works");
-	return test_done();
+	cl_assert_equal_i(count, ARRAY_SIZE(key_val));
+	cl_assert_equal_i(hashmap_get_size(&map.map), ARRAY_SIZE(key_val));
 }
-- 
2.47.0.86.g15030f9556

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

[Seyi Kuforiji]

Adapt oidtree test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
before parsing. This prevents issues from an uninitialized or invalid
hash algorithm.

Introduce 'test_oidtree__initialize` handles the to set up of the global
oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
tests are completed.

With this change, `check_each` stops at the first error encountered,
making it easier to address it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                  |  2 +-
 t/meson.build                             |  2 +-
 t/unit-tests/{t-oidtree.c => u-oidtree.c} | 79 +++++++++--------------
 3 files changed, 34 insertions(+), 49 deletions(-)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (45%)

diff --git a/Makefile b/Makefile
index e4e85e6007..2b134efc70 100644
--- a/Makefile
+++ b/Makefile
@@ -1358,6 +1358,7 @@ CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 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-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1369,7 +1370,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-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
diff --git a/t/meson.build b/t/meson.build
index d5b83cdb72..91699917ff 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -6,6 +6,7 @@ clar_test_suites = [
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
   'unit-tests/u-oidmap.c',
+  'unit-tests/u-oidtree.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -51,7 +52,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
   'unit-tests/t-reftable-merged.c',
diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/u-oidtree.c
similarity index 45%
rename from t/unit-tests/t-oidtree.c
rename to t/unit-tests/u-oidtree.c
index a38754b066..e6eede2740 100644
--- a/t/unit-tests/t-oidtree.c
+++ b/t/unit-tests/u-oidtree.c
@@ -1,10 +1,12 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oidtree.h"
 #include "hash.h"
 #include "hex.h"
 #include "strvec.h"
 
+static struct oidtree ot;
+
 #define FILL_TREE(tree, ...)                                       \
 	do {                                                       \
 		const char *hexes[] = { __VA_ARGS__ };             \
@@ -16,8 +18,7 @@ static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oidtree_insert(ot, &oid);
 	}
 	return 0;
@@ -27,10 +28,8 @@ static void check_contains(struct oidtree *ot, const char *hex, int expected)
 {
 	struct object_id oid;
 
-	if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
-		return;
-	if (!check_int(oidtree_contains(ot, &oid), ==, expected))
-		test_msg("oid: %s", oid_to_hex(&oid));
+	cl_parse_any_oid(hex, &oid);
+	cl_assert_equal_i(oidtree_contains(ot, &oid), expected);
 }
 
 struct expected_hex_iter {
@@ -44,19 +43,11 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)
 	struct expected_hex_iter *hex_iter = data;
 	struct object_id expected;
 
-	if (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {
-		test_msg("error: extraneous callback for query: ('%s'), object_id: ('%s')",
-			 hex_iter->query, oid_to_hex(oid));
-		return CB_BREAK;
-	}
-
-	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
-					     &expected), ==, 0))
-		; /* the data is bogus and cannot be used */
-	else if (!check(oideq(oid, &expected)))
-		test_msg("expected: %s\n       got: %s\n     query: %s",
-			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
+	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
 
+	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
+			 &expected);
+	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));
 	hex_iter->i += 1;
 	return CB_CONTINUE;
 }
@@ -75,48 +66,42 @@ static void check_each(struct oidtree *ot, const char *query, ...)
 		strvec_push(&hex_iter.expected_hexes, arg);
 	va_end(hex_args);
 
-	if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
-		return;
+	cl_parse_any_oid(query, &oid);
 	oidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);
 
-	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
-		test_msg("error: could not find some 'object_id's for query ('%s')", query);
+	if (hex_iter.i != hex_iter.expected_hexes.nr)
+		cl_failf("error: could not find some 'object_id's for query ('%s')", query);
+
 	strvec_clear(&hex_iter.expected_hexes);
 }
 
-static void setup(void (*f)(struct oidtree *ot))
+void test_oidtree__initialize(void)
 {
-	struct oidtree ot;
-
 	oidtree_init(&ot);
-	f(&ot);
-	oidtree_clear(&ot);
 }
 
-static void t_contains(struct oidtree *ot)
+void test_oidtree__cleanup(void)
 {
-	FILL_TREE(ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
-	check_contains(ot, "44", 0);
-	check_contains(ot, "441", 0);
-	check_contains(ot, "440", 0);
-	check_contains(ot, "444", 1);
-	check_contains(ot, "4440", 1);
-	check_contains(ot, "4444", 0);
+	oidtree_clear(&ot);
 }
 
-static void t_each(struct oidtree *ot)
+void test_oidtree__contains(void)
 {
-	FILL_TREE(ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
-	check_each(ot, "12300", "123", NULL);
-	check_each(ot, "3211", NULL); /* should not reach callback */
-	check_each(ot, "3210", "321", NULL);
-	check_each(ot, "32100", "321", NULL);
-	check_each(ot, "32", "320", "321", NULL);
+	FILL_TREE(&ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
+	check_contains(&ot, "44", 0);
+	check_contains(&ot, "441", 0);
+	check_contains(&ot, "440", 0);
+	check_contains(&ot, "444", 1);
+	check_contains(&ot, "4440", 1);
+	check_contains(&ot, "4444", 0);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_oidtree__each(void)
 {
-	TEST(setup(t_contains), "oidtree insert and contains works");
-	TEST(setup(t_each), "oidtree each works");
-	return test_done();
+	FILL_TREE(&ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
+	check_each(&ot, "12300", "123", NULL);
+	check_each(&ot, "3211", NULL); /* should not reach callback */
+	check_each(&ot, "3210", "321", NULL);
+	check_each(&ot, "32100", "321", NULL);
+	check_each(&ot, "32", "320", "321", NULL);
 }
-- 
2.47.0.86.g15030f9556

Re: [PATCH v2 1/4] t/unit-tests: implement clar specific oid helper functions

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> -static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
> +static void cl_parse_oid(const char *hex, struct object_id *oid,
>  				       const struct git_hash_algo *algop)
>  {
>  	int ret;
>  	size_t sz = strlen(hex);
>  	struct strbuf buf = STRBUF_INIT;
>  
> -	if (!check(sz <= algop->hexsz)) {
> -		test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
> -			 hex, (unsigned long)algop->hexsz);
> -		return -1;
> -	}
> +	cl_assert(sz <= algop->hexsz);
>  
>  	strbuf_add(&buf, hex, sz);
>  	strbuf_addchars(&buf, '0', algop->hexsz - sz);
>  
> -	ret = get_oid_hex_algop(buf.buf, oid, algop);
> -	if (!check_int(ret, ==, 0))
> -		test_msg("BUG: invalid hex input (%s) provided", hex);
> +	cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
>  
>  	strbuf_release(&buf);
> -	return ret;
>  }

As you are not returning "ret" and making the function void, you
made "int ret" an unused variable that needs removing.

Thanks.

Re: [PATCH v2 1/4] t/unit-tests: implement clar specific oid helper functions

[Seyi Chamber]

On Mon, 24 Feb 2025 at 18:56, Junio C Hamano <gitster@pobox.com> wrote:
>
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
>
> > -static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
> > +static void cl_parse_oid(const char *hex, struct object_id *oid,
> >                                      const struct git_hash_algo *algop)
> >  {
> >       int ret;
> >       size_t sz = strlen(hex);
> >       struct strbuf buf = STRBUF_INIT;
> >
> > -     if (!check(sz <= algop->hexsz)) {
> > -             test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
> > -                      hex, (unsigned long)algop->hexsz);
> > -             return -1;
> > -     }
> > +     cl_assert(sz <= algop->hexsz);
> >
> >       strbuf_add(&buf, hex, sz);
> >       strbuf_addchars(&buf, '0', algop->hexsz - sz);
> >
> > -     ret = get_oid_hex_algop(buf.buf, oid, algop);
> > -     if (!check_int(ret, ==, 0))
> > -             test_msg("BUG: invalid hex input (%s) provided", hex);
> > +     cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
> >
> >       strbuf_release(&buf);
> > -     return ret;
> >  }
>
> As you are not returning "ret" and making the function void, you
> made "int ret" an unused variable that needs removing.
>
> Thanks.

Hi Junio,

Do I send in a new patch series addressing this?

Thanks,
Seyi

Re: [PATCH v2 1/4] t/unit-tests: implement clar specific oid helper functions

[Patrick Steinhardt]

On Tue, Feb 25, 2025 at 08:14:35AM +0100, Seyi Chamber wrote:
> On Mon, 24 Feb 2025 at 18:56, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Seyi Kuforiji <kuforiji98@gmail.com> writes:
> >
> > > -static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
> > > +static void cl_parse_oid(const char *hex, struct object_id *oid,
> > >                                      const struct git_hash_algo *algop)
> > >  {
> > >       int ret;
> > >       size_t sz = strlen(hex);
> > >       struct strbuf buf = STRBUF_INIT;
> > >
> > > -     if (!check(sz <= algop->hexsz)) {
> > > -             test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
> > > -                      hex, (unsigned long)algop->hexsz);
> > > -             return -1;
> > > -     }
> > > +     cl_assert(sz <= algop->hexsz);
> > >
> > >       strbuf_add(&buf, hex, sz);
> > >       strbuf_addchars(&buf, '0', algop->hexsz - sz);
> > >
> > > -     ret = get_oid_hex_algop(buf.buf, oid, algop);
> > > -     if (!check_int(ret, ==, 0))
> > > -             test_msg("BUG: invalid hex input (%s) provided", hex);
> > > +     cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
> > >
> > >       strbuf_release(&buf);
> > > -     return ret;
> > >  }
> >
> > As you are not returning "ret" and making the function void, you
> > made "int ret" an unused variable that needs removing.
> >
> > Thanks.
> 
> Hi Junio,
> 
> Do I send in a new patch series addressing this?

The expectation would be that you reroll the series and send out a new
version thereof that gets rid of the return value in the same commit
where you stop returning it.

This would typically be the case as long as your series has not yet been
merged to `next`. Once it has been merged to `next`, you would have to
send an entirely new patch series as a follow-up.

Patrick

[PATCH v3 0/4] t/unit-tests: convert unit-tests to use clar

[Seyi Kuforiji]

Hello,

This small patch series transitions a couple more of our existing unit
test files to the Clar testing framework. This change is part of our
ongoing effort to standardize our testing framework to enhance
maintainability.

Changes in v3:
 - minor code change based on review

Thanks
Seyi

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Philip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>


Seyi Kuforiji (4):
  t/unit-tests: implement clar specific oid helper functions
  t/unit-tests: convert oid-array test to use clar test framework
  t/unit-tests: convert oidmap test to use clar test framework
  t/unit-tests: convert oidtree test to use clar test framework

 Makefile                                      |   8 +-
 t/meson.build                                 |   8 +-
 t/unit-tests/lib-oid.c                        |  32 ++--
 t/unit-tests/lib-oid.h                        |   9 +-
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 125 +++++++-------
 t/unit-tests/{t-oidmap.c => u-oidmap.c}       | 153 +++++++-----------
 t/unit-tests/{t-oidtree.c => u-oidtree.c}     |  79 ++++-----
 t/unit-tests/unit-test.c                      |   2 +
 8 files changed, 177 insertions(+), 239 deletions(-)
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (34%)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (45%)

Range-diff against v2:
1:  7f14d0d574 ! 1:  c5b6613617 t/unit-tests: implement clar specific oid helper functions
    @@ t/unit-tests/lib-oid.c: int init_hash_algo(void)
     +static void cl_parse_oid(const char *hex, struct object_id *oid,
      				       const struct git_hash_algo *algop)
      {
    - 	int ret;
    +-	int ret;
      	size_t sz = strlen(hex);
      	struct strbuf buf = STRBUF_INIT;
      
2:  430f5c5007 = 2:  d6cc4985a6 t/unit-tests: convert oid-array test to use clar test framework
3:  319cea1265 = 3:  1087752df5 t/unit-tests: convert oidmap test to use clar test framework
4:  ea63a5c9f1 = 4:  cda8dc194c t/unit-tests: convert oidtree test to use clar test framework
-- 
2.47.0.86.g15030f9556

[PATCH v3 1/4] t/unit-tests: implement clar specific oid helper functions

[Seyi Kuforiji]

`get_oid_arbitrary_hex()` and `init_hash_algo()` are both required for
oid-related tests to run without errors. In the current implementation,
both functions are defined and declared in the
`t/unit-tests/lib-oid.{c,h}` which is utilized by oid-related tests in
the homegrown unit tests structure.

Adapt functions in lib-oid.{c,h} to use clar. Both these functions
become available for oid-related test files implemented using the clar
testing framework, which requires them. This will be used by subsequent
commits.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                 |  2 +-
 t/meson.build            |  2 +-
 t/unit-tests/lib-oid.c   | 32 +++++++++++---------------------
 t/unit-tests/lib-oid.h   |  9 ++++++---
 t/unit-tests/unit-test.c |  2 ++
 5 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/Makefile b/Makefile
index bcf5ed3f85..81799488f0 100644
--- a/Makefile
+++ b/Makefile
@@ -1365,6 +1365,7 @@ CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 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-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
@@ -1381,7 +1382,6 @@ UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
-UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
diff --git a/t/meson.build b/t/meson.build
index 780939d49f..862cf1cfd4 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -14,6 +14,7 @@ clar_test_suites = [
 clar_sources = [
   'unit-tests/clar/clar.c',
   'unit-tests/unit-test.c',
+  'unit-tests/lib-oid.c'
 ]
 
 clar_decls_h = custom_target(
@@ -68,7 +69,6 @@ foreach unit_test_program : unit_test_programs
   unit_test = executable(unit_test_name,
     sources: [
       'unit-tests/test-lib.c',
-      'unit-tests/lib-oid.c',
       'unit-tests/lib-reftable.c',
       unit_test_program,
     ],
diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c
index 8f0ccac532..e0b3180f23 100644
--- a/t/unit-tests/lib-oid.c
+++ b/t/unit-tests/lib-oid.c
@@ -1,9 +1,9 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "strbuf.h"
 #include "hex.h"
 
-int init_hash_algo(void)
+int cl_setup_hash_algo(void)
 {
 	static int algo = -1;
 
@@ -11,42 +11,32 @@ int init_hash_algo(void)
 		const char *algo_name = getenv("GIT_TEST_DEFAULT_HASH");
 		algo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;
 
-		if (!check(algo != GIT_HASH_UNKNOWN))
-			test_msg("BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')",
-				 algo_name);
+		cl_assert(algo != GIT_HASH_UNKNOWN);
 	}
 	return algo;
 }
 
-static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,
+static void cl_parse_oid(const char *hex, struct object_id *oid,
 				       const struct git_hash_algo *algop)
 {
-	int ret;
 	size_t sz = strlen(hex);
 	struct strbuf buf = STRBUF_INIT;
 
-	if (!check(sz <= algop->hexsz)) {
-		test_msg("BUG: hex string (%s) bigger than maximum allowed (%lu)",
-			 hex, (unsigned long)algop->hexsz);
-		return -1;
-	}
+	cl_assert(sz <= algop->hexsz);
 
 	strbuf_add(&buf, hex, sz);
 	strbuf_addchars(&buf, '0', algop->hexsz - sz);
 
-	ret = get_oid_hex_algop(buf.buf, oid, algop);
-	if (!check_int(ret, ==, 0))
-		test_msg("BUG: invalid hex input (%s) provided", hex);
+	cl_assert_equal_i(get_oid_hex_algop(buf.buf, oid, algop), 0);
 
 	strbuf_release(&buf);
-	return ret;
 }
 
-int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)
+
+void cl_parse_any_oid(const char *hex, struct object_id *oid)
 {
-	int hash_algo = init_hash_algo();
+	int hash_algo = cl_setup_hash_algo();
 
-	if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))
-		return -1;
-	return get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);
+	cl_assert(hash_algo != GIT_HASH_UNKNOWN);
+	cl_parse_oid(hex, oid, &hash_algos[hash_algo]);
 }
diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
index 4e77c04bd2..4031775104 100644
--- a/t/unit-tests/lib-oid.h
+++ b/t/unit-tests/lib-oid.h
@@ -5,6 +5,7 @@
 
 /*
  * Convert arbitrary hex string to object_id.
+ *
  * For example, passing "abc12" will generate
  * "abc1200000000000000000000000000000000000" hex of length 40 for SHA-1 and
  * create object_id with that.
@@ -12,14 +13,16 @@
  * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH
  * environment variable.
  */
-int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
+
+void cl_parse_any_oid (const char *s, struct object_id *oid);
 /*
  * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
  * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the
  * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
- * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the
+ * cl_assert(algo != GIT_HASH_UNKNOWN) before returning to verify if the
  * GIT_TEST_DEFAULT_HASH's value is valid or not.
  */
-int init_hash_algo(void);
+
+int cl_setup_hash_algo(void);
 
 #endif /* LIB_OID_H */
diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
index fa8818842a..5af645048a 100644
--- a/t/unit-tests/unit-test.c
+++ b/t/unit-tests/unit-test.c
@@ -1,5 +1,7 @@
 #include "unit-test.h"
+#include "hex.h"
 #include "parse-options.h"
+#include "strbuf.h"
 #include "string-list.h"
 #include "strvec.h"
 
-- 
2.47.0.86.g15030f9556

[PATCH v3 2/4] t/unit-tests: convert oid-array test to use clar test framework

[Seyi Kuforiji]

Adapt oid-array test script to clar framework by using clar assertions
where necessary. Remove descriptions from macros to reduce
redundancy, and move test input arrays to global scope for reuse across
multiple test functions. Introduce `test_oid_array__initialize()` to
explicitly initialize the hash algorithm.

These changes streamline the test suite, making individual tests
self-contained and reducing redundant code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                      |   2 +-
 t/meson.build                                 |   2 +-
 t/unit-tests/{t-oid-array.c => u-oid-array.c} | 125 +++++++++---------
 3 files changed, 66 insertions(+), 63 deletions(-)
 rename t/unit-tests/{t-oid-array.c => u-oid-array.c} (34%)

diff --git a/Makefile b/Makefile
index 81799488f0..c9b0320fcb 100644
--- a/Makefile
+++ b/Makefile
@@ -1356,6 +1356,7 @@ CLAR_TEST_SUITES += u-example-decorate
 CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
+CLAR_TEST_SUITES += u-oid-array
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1367,7 +1368,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-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
diff --git a/t/meson.build b/t/meson.build
index 862cf1cfd4..1b17a53acc 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -4,6 +4,7 @@ clar_test_suites = [
   'unit-tests/u-hash.c',
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
+  'unit-tests/u-oid-array.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -49,7 +50,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/u-oid-array.c
similarity index 34%
rename from t/unit-tests/t-oid-array.c
rename to t/unit-tests/u-oid-array.c
index 45b59a2a51..e48a433f21 100644
--- a/t/unit-tests/t-oid-array.c
+++ b/t/unit-tests/u-oid-array.c
@@ -1,22 +1,19 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oid-array.h"
 #include "hex.h"
 
-static int fill_array(struct oid_array *array, const char *hexes[], size_t n)
+static void fill_array(struct oid_array *array, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
 
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oid_array_append(array, &oid);
 	}
-	if (!check_uint(array->nr, ==, n))
-		return -1;
-	return 0;
+	cl_assert_equal_i(array->nr, n);
 }
 
 static int add_to_oid_array(const struct object_id *oid, void *data)
@@ -34,30 +31,22 @@ static void t_enumeration(const char **input_args, size_t input_sz,
 			 actual = OID_ARRAY_INIT;
 	size_t i;
 
-	if (fill_array(&input, input_args, input_sz))
-		return;
-	if (fill_array(&expect, expect_args, expect_sz))
-		return;
+	fill_array(&input, input_args, input_sz);
+	fill_array(&expect, expect_args, expect_sz);
 
 	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
-	if (!check_uint(actual.nr, ==, expect.nr))
-		return;
-
-	for (i = 0; i < actual.nr; i++) {
-		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
-			test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
-				 oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
-				 (uintmax_t)i);
-	}
+	cl_assert_equal_i(actual.nr, expect.nr);
+
+	for (i = 0; i < actual.nr; i++)
+		cl_assert(oideq(&actual.oid[i], &expect.oid[i]));
 
 	oid_array_clear(&actual);
 	oid_array_clear(&input);
 	oid_array_clear(&expect);
 }
 
-#define TEST_ENUMERATION(input, expect, desc)                                     \
-	TEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \
-			   desc " works")
+#define TEST_ENUMERATION(input, expect)                                     \
+	t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect));
 
 static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 		     int lower_bound, int upper_bound)
@@ -66,61 +55,75 @@ static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
 	struct object_id oid_query;
 	int ret;
 
-	if (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))
-		return;
-	if (fill_array(&array, input_hexes, n))
-		return;
+	cl_parse_any_oid(query_hex, &oid_query);
+	fill_array(&array, input_hexes, n);
 	ret = oid_array_lookup(&array, &oid_query);
 
-	if (!check_int(ret, <=, upper_bound) ||
-	    !check_int(ret, >=, lower_bound))
-		test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
+	cl_assert(ret <= upper_bound);
+	cl_assert(ret >= lower_bound);
 
 	oid_array_clear(&array);
 }
 
-#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \
-	TEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
-		      lower_bound, upper_bound),                        \
-	     desc " works")
+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound) \
+	t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query,      \
+		      lower_bound, upper_bound);
 
-static void setup(void)
+void test_oid_array__initialize(void)
 {
 	/* The hash algo is used by oid_array_lookup() internally */
-	int algo = init_hash_algo();
-	if (check_int(algo, !=, GIT_HASH_UNKNOWN))
-		repo_set_hash_algo(the_repository, algo);
+	int algo = cl_setup_hash_algo();
+	repo_set_hash_algo(the_repository, algo);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+static const char *arr_input[] = { "88", "44", "aa", "55" };
+static const char *arr_input_dup[] = { "88", "44", "aa", "55",
+				       "88", "44", "aa", "55",
+				       "88", "44", "aa", "55" };
+static const char *res_sorted[] = { "44", "55", "88", "aa" };
+
+void test_oid_array__enumerate_unique(void)
 {
-	const char *arr_input[] = { "88", "44", "aa", "55" };
-	const char *arr_input_dup[] = { "88", "44", "aa", "55",
-					"88", "44", "aa", "55",
-					"88", "44", "aa", "55" };
-	const char *res_sorted[] = { "44", "55", "88", "aa" };
-	const char *nearly_55;
+	TEST_ENUMERATION(arr_input, res_sorted);
+}
+
+void test_oid_array__enumerate_duplicate(void)
+{
+	TEST_ENUMERATION(arr_input_dup, res_sorted);
+}
+
+void test_oid_array__lookup(void)
+{
+	TEST_LOOKUP(arr_input, "55", 1, 1);
+}
 
-	if (!TEST(setup(), "setup"))
-		test_skip_all("hash algo initialization failed");
+void test_oid_array__lookup_non_existent(void)
+{
+	TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
+}
+
+void test_oid_array__lookup_duplicates(void)
+{
+	TEST_LOOKUP(arr_input_dup, "55", 3, 5);
+}
 
-	TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
-	TEST_ENUMERATION(arr_input_dup, res_sorted,
-			 "ordered enumeration with duplicate suppression");
+void test_oid_array__lookup_non_existent_dup(void)
+{
+	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
+}
 
-	TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
-	TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
-	TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
-	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
-		    "lookup non-existent entry with duplicates");
+void test_oid_array__lookup_almost_dup(void)
+{
+	const char *nearly_55;
 
-	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
+	nearly_55 = cl_setup_hash_algo() == GIT_HASH_SHA1 ?
 			"5500000000000000000000000000000000000001" :
 			"5500000000000000000000000000000000000000000000000000000000000001";
-	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
-		    "lookup with almost duplicate values");
-	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
-		    "lookup with single duplicate value");
 
-	return test_done();
+	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
+}
+
+void test_oid_array__lookup_single_dup(void)
+{
+	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
 }
-- 
2.47.0.86.g15030f9556

[PATCH v3 3/4] t/unit-tests: convert oidmap test to use clar test framework

[Seyi Kuforiji]

Adapt oidmap test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
before parsing. This prevents issues from an uninitialized or invalid
hash algorithm.

Introduce 'test_oidmap__initialize` handles the to set up of the global
oidmap map with predefined key-value pairs, and `test_oidmap__cleanup`
frees the oidmap and its entries when all tests are completed.

The test loops through all entries to detect multiple errors. With this
change, it stops at the first error encountered, making it easier to
address it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                |   2 +-
 t/meson.build                           |   2 +-
 t/unit-tests/{t-oidmap.c => u-oidmap.c} | 153 +++++++++---------------
 3 files changed, 56 insertions(+), 101 deletions(-)
 rename t/unit-tests/{t-oidmap.c => u-oidmap.c} (32%)

diff --git a/Makefile b/Makefile
index c9b0320fcb..e4e85e6007 100644
--- a/Makefile
+++ b/Makefile
@@ -1357,6 +1357,7 @@ CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-oid-array
+CLAR_TEST_SUITES += u-oidmap
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1368,7 +1369,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-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
diff --git a/t/meson.build b/t/meson.build
index 1b17a53acc..d5b83cdb72 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -5,6 +5,7 @@ clar_test_suites = [
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
+  'unit-tests/u-oidmap.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -50,7 +51,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/u-oidmap.c
similarity index 32%
rename from t/unit-tests/t-oidmap.c
rename to t/unit-tests/u-oidmap.c
index b22e52d08b..dc805b7e3c 100644
--- a/t/unit-tests/t-oidmap.c
+++ b/t/unit-tests/u-oidmap.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oidmap.h"
 #include "hash.h"
@@ -18,102 +18,85 @@ static const char *const key_val[][2] = { { "11", "one" },
 					  { "22", "two" },
 					  { "33", "three" } };
 
-static void setup(void (*f)(struct oidmap *map))
+static struct oidmap map;
+
+void test_oidmap__initialize(void)
 {
-	struct oidmap map = OIDMAP_INIT;
-	int ret = 0;
+	oidmap_init(&map, 0);
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++){
 		struct test_entry *entry;
 
 		FLEX_ALLOC_STR(entry, name, key_val[i][1]);
-		if ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) {
-			free(entry);
-			break;
-		}
-		entry = oidmap_put(&map, entry);
-		if (!check(entry == NULL))
-			free(entry);
+		cl_parse_any_oid(key_val[i][0], &entry->entry.oid);
+		cl_assert(oidmap_put(&map, entry) == NULL);
 	}
+}
 
-	if (!ret)
-		f(&map);
+void test_oidmap__cleanup(void)
+{
 	oidmap_free(&map, 1);
 }
 
-static void t_replace(struct oidmap *map)
+void test_oidmap__replace(void)
 {
 	struct test_entry *entry, *prev;
 
 	FLEX_ALLOC_STR(entry, name, "un");
-	if (get_oid_arbitrary_hex("11", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "one");
+	cl_parse_any_oid("11", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "one");
 	free(prev);
 
 	FLEX_ALLOC_STR(entry, name, "deux");
-	if (get_oid_arbitrary_hex("22", &entry->entry.oid))
-		return;
-	prev = oidmap_put(map, entry);
-	if (!check(prev != NULL))
-		return;
-	check_str(prev->name, "two");
+	cl_parse_any_oid("22", &entry->entry.oid);
+	prev = oidmap_put(&map, entry);
+	cl_assert(prev != NULL);
+	cl_assert_equal_s(prev->name, "two");
 	free(prev);
 }
 
-static void t_get(struct oidmap *map)
+void test_oidmap__get(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_get(map, &oid) == NULL);
-
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_get(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_get(&map, &oid) == NULL);
+
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_get(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
 }
 
-static void t_remove(struct oidmap *map)
+void test_oidmap__remove(void)
 {
 	struct test_entry *entry;
 	struct object_id oid;
 
-	if (get_oid_arbitrary_hex("11", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "one");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("11", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "one");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("22", &oid))
-		return;
-	entry = oidmap_remove(map, &oid);
-	if (!check(entry != NULL))
-		return;
-	check_str(entry->name, "two");
-	check(oidmap_get(map, &oid) == NULL);
+	cl_parse_any_oid("22", &oid);
+	entry = oidmap_remove(&map, &oid);
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(entry->name, "two");
+	cl_assert(oidmap_get(&map, &oid) == NULL);
 	free(entry);
 
-	if (get_oid_arbitrary_hex("44", &oid))
-		return;
-	check(oidmap_remove(map, &oid) == NULL);
+	cl_parse_any_oid("44", &oid);
+	cl_assert(oidmap_remove(&map, &oid) == NULL);
 }
 
 static int key_val_contains(struct test_entry *entry, char seen[])
@@ -121,8 +104,7 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		struct object_id oid;
 
-		if (get_oid_arbitrary_hex(key_val[i][0], &oid))
-			return -1;
+		cl_parse_any_oid(key_val[i][0], &oid);
 
 		if (oideq(&entry->entry.oid, &oid)) {
 			if (seen[i])
@@ -134,48 +116,21 @@ static int key_val_contains(struct test_entry *entry, char seen[])
 	return 1;
 }
 
-static void t_iterate(struct oidmap *map)
+void test_oidmap__iterate(void)
 {
 	struct oidmap_iter iter;
 	struct test_entry *entry;
 	char seen[ARRAY_SIZE(key_val)] = { 0 };
 	int count = 0;
 
-	oidmap_iter_init(map, &iter);
+	oidmap_iter_init(&map, &iter);
 	while ((entry = oidmap_iter_next(&iter))) {
-		int ret;
-		if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {
-			switch (ret) {
-			case -1:
-				break; /* error message handled by get_oid_arbitrary_hex() */
-			case 1:
-				test_msg("obtained entry was not given in the input\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			case 2:
-				test_msg("duplicate entry detected\n"
-					 "  name: %s\n   oid: %s\n",
-					 entry->name, oid_to_hex(&entry->entry.oid));
-				break;
-			default:
-				test_msg("BUG: invalid return value (%d) from key_val_contains()",
-					 ret);
-				break;
-			}
-		} else {
-			count++;
+		if (key_val_contains(entry, seen) != 0) {
+			cl_failf("Unexpected entry: name = %s, oid = %s",
+				 entry->name, oid_to_hex(&entry->entry.oid));
 		}
+		count++;
 	}
-	check_int(count, ==, ARRAY_SIZE(key_val));
-	check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));
-}
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST(setup(t_replace), "replace works");
-	TEST(setup(t_get), "get works");
-	TEST(setup(t_remove), "remove works");
-	TEST(setup(t_iterate), "iterate works");
-	return test_done();
+	cl_assert_equal_i(count, ARRAY_SIZE(key_val));
+	cl_assert_equal_i(hashmap_get_size(&map.map), ARRAY_SIZE(key_val));
 }
-- 
2.47.0.86.g15030f9556

[PATCH v3 4/4] t/unit-tests: convert oidtree test to use clar test framework

[Seyi Kuforiji]

Adapt oidtree test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid()` ensures the hash algorithm is set
before parsing. This prevents issues from an uninitialized or invalid
hash algorithm.

Introduce 'test_oidtree__initialize` handles the to set up of the global
oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
tests are completed.

With this change, `check_each` stops at the first error encountered,
making it easier to address it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 Makefile                                  |  2 +-
 t/meson.build                             |  2 +-
 t/unit-tests/{t-oidtree.c => u-oidtree.c} | 79 +++++++++--------------
 3 files changed, 34 insertions(+), 49 deletions(-)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (45%)

diff --git a/Makefile b/Makefile
index e4e85e6007..2b134efc70 100644
--- a/Makefile
+++ b/Makefile
@@ -1358,6 +1358,7 @@ CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 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-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1369,7 +1370,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-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
diff --git a/t/meson.build b/t/meson.build
index d5b83cdb72..91699917ff 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -6,6 +6,7 @@ clar_test_suites = [
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-oid-array.c',
   'unit-tests/u-oidmap.c',
+  'unit-tests/u-oidtree.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -51,7 +52,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
   'unit-tests/t-reftable-merged.c',
diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/u-oidtree.c
similarity index 45%
rename from t/unit-tests/t-oidtree.c
rename to t/unit-tests/u-oidtree.c
index a38754b066..e6eede2740 100644
--- a/t/unit-tests/t-oidtree.c
+++ b/t/unit-tests/u-oidtree.c
@@ -1,10 +1,12 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "lib-oid.h"
 #include "oidtree.h"
 #include "hash.h"
 #include "hex.h"
 #include "strvec.h"
 
+static struct oidtree ot;
+
 #define FILL_TREE(tree, ...)                                       \
 	do {                                                       \
 		const char *hexes[] = { __VA_ARGS__ };             \
@@ -16,8 +18,7 @@ static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oidtree_insert(ot, &oid);
 	}
 	return 0;
@@ -27,10 +28,8 @@ static void check_contains(struct oidtree *ot, const char *hex, int expected)
 {
 	struct object_id oid;
 
-	if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
-		return;
-	if (!check_int(oidtree_contains(ot, &oid), ==, expected))
-		test_msg("oid: %s", oid_to_hex(&oid));
+	cl_parse_any_oid(hex, &oid);
+	cl_assert_equal_i(oidtree_contains(ot, &oid), expected);
 }
 
 struct expected_hex_iter {
@@ -44,19 +43,11 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)
 	struct expected_hex_iter *hex_iter = data;
 	struct object_id expected;
 
-	if (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {
-		test_msg("error: extraneous callback for query: ('%s'), object_id: ('%s')",
-			 hex_iter->query, oid_to_hex(oid));
-		return CB_BREAK;
-	}
-
-	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
-					     &expected), ==, 0))
-		; /* the data is bogus and cannot be used */
-	else if (!check(oideq(oid, &expected)))
-		test_msg("expected: %s\n       got: %s\n     query: %s",
-			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
+	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
 
+	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
+			 &expected);
+	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));
 	hex_iter->i += 1;
 	return CB_CONTINUE;
 }
@@ -75,48 +66,42 @@ static void check_each(struct oidtree *ot, const char *query, ...)
 		strvec_push(&hex_iter.expected_hexes, arg);
 	va_end(hex_args);
 
-	if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
-		return;
+	cl_parse_any_oid(query, &oid);
 	oidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);
 
-	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
-		test_msg("error: could not find some 'object_id's for query ('%s')", query);
+	if (hex_iter.i != hex_iter.expected_hexes.nr)
+		cl_failf("error: could not find some 'object_id's for query ('%s')", query);
+
 	strvec_clear(&hex_iter.expected_hexes);
 }
 
-static void setup(void (*f)(struct oidtree *ot))
+void test_oidtree__initialize(void)
 {
-	struct oidtree ot;
-
 	oidtree_init(&ot);
-	f(&ot);
-	oidtree_clear(&ot);
 }
 
-static void t_contains(struct oidtree *ot)
+void test_oidtree__cleanup(void)
 {
-	FILL_TREE(ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
-	check_contains(ot, "44", 0);
-	check_contains(ot, "441", 0);
-	check_contains(ot, "440", 0);
-	check_contains(ot, "444", 1);
-	check_contains(ot, "4440", 1);
-	check_contains(ot, "4444", 0);
+	oidtree_clear(&ot);
 }
 
-static void t_each(struct oidtree *ot)
+void test_oidtree__contains(void)
 {
-	FILL_TREE(ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
-	check_each(ot, "12300", "123", NULL);
-	check_each(ot, "3211", NULL); /* should not reach callback */
-	check_each(ot, "3210", "321", NULL);
-	check_each(ot, "32100", "321", NULL);
-	check_each(ot, "32", "320", "321", NULL);
+	FILL_TREE(&ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
+	check_contains(&ot, "44", 0);
+	check_contains(&ot, "441", 0);
+	check_contains(&ot, "440", 0);
+	check_contains(&ot, "444", 1);
+	check_contains(&ot, "4440", 1);
+	check_contains(&ot, "4444", 0);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_oidtree__each(void)
 {
-	TEST(setup(t_contains), "oidtree insert and contains works");
-	TEST(setup(t_each), "oidtree each works");
-	return test_done();
+	FILL_TREE(&ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
+	check_each(&ot, "12300", "123", NULL);
+	check_each(&ot, "3211", NULL); /* should not reach callback */
+	check_each(&ot, "3210", "321", NULL);
+	check_each(&ot, "32100", "321", NULL);
+	check_each(&ot, "32", "320", "321", NULL);
 }
-- 
2.47.0.86.g15030f9556