diff mbox series

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

Message ID 20250220082959.10854-3-kuforiji98@gmail.com (mailing list archive)
State New
Headers show
Series t/unit-tests: convert unit-tests to use clar | expand

Commit Message

Seyi Kuforiji Feb. 20, 2025, 8:29 a.m. UTC
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%)

Comments

Phillip Wood Feb. 20, 2025, 2:38 p.m. UTC | #1
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);
>   }
diff mbox series

Patch

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);
 }