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

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

[Seyi Kuforiji]

Hello,

This small patch series transitions the 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 (4):
  t/unit-tests: convert hashmap test to use clar test framework
  t/unit-tests: adapt example decorate test to use clar test framework
  t/unit-tests: convert strbuf test to use clar test framework
  t/unit-tests: convert strcmp-offset test to use clar test framework

 Makefile                                  |   8 +-
 t/meson.build                             |   8 +-
 t/unit-tests/t-example-decorate.c         |  74 -------
 t/unit-tests/t-strbuf.c                   | 122 ------------
 t/unit-tests/t-strcmp-offset.c            |  35 ----
 t/unit-tests/u-example-decorate.c         |  76 ++++++++
 t/unit-tests/{t-hashmap.c => u-hashmap.c} | 226 +++++++++++-----------
 t/unit-tests/u-strbuf.c                   | 121 ++++++++++++
 t/unit-tests/u-strcmp-offset.c            |  45 +++++
 9 files changed, 362 insertions(+), 353 deletions(-)
 delete mode 100644 t/unit-tests/t-example-decorate.c
 delete mode 100644 t/unit-tests/t-strbuf.c
 delete mode 100644 t/unit-tests/t-strcmp-offset.c
 create mode 100644 t/unit-tests/u-example-decorate.c
 rename t/unit-tests/{t-hashmap.c => u-hashmap.c} (60%)
 create mode 100644 t/unit-tests/u-strbuf.c
 create mode 100644 t/unit-tests/u-strcmp-offset.c

-- 
2.47.0.86.g15030f9556

[PATCH 1/4] t/unit-tests: convert hashmap test to clar framework

[Seyi Kuforiji]

Adapts hashmap test script to clar framework by using clar assertions
where necessary. Test functions are created as both standalone and
inline to test different test cases.

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-hashmap.c => u-hashmap.c} | 226 +++++++++++-----------
 3 files changed, 114 insertions(+), 116 deletions(-)
 rename t/unit-tests/{t-hashmap.c => u-hashmap.c} (60%)

diff --git a/Makefile b/Makefile
index b0cdc1fd4a..2d9dad119a 100644
--- a/Makefile
+++ b/Makefile
@@ -1339,6 +1339,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
 CLAR_TEST_SUITES += u-ctype
 CLAR_TEST_SUITES += u-hash
+CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
@@ -1349,7 +1350,6 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
 UNIT_TEST_PROGRAMS += t-example-decorate
-UNIT_TEST_PROGRAMS += t-hashmap
 UNIT_TEST_PROGRAMS += t-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
diff --git a/t/meson.build b/t/meson.build
index 14fea8dddf..af597f9804 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,6 +1,7 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
   'unit-tests/u-hash.c',
+  'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
@@ -45,7 +46,6 @@ test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
   'unit-tests/t-example-decorate.c',
-  'unit-tests/t-hashmap.c',
   'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
diff --git a/t/unit-tests/t-hashmap.c b/t/unit-tests/u-hashmap.c
similarity index 60%
rename from t/unit-tests/t-hashmap.c
rename to t/unit-tests/u-hashmap.c
index 83b79dff39..6b6d22005a 100644
--- a/t/unit-tests/t-hashmap.c
+++ b/t/unit-tests/u-hashmap.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "hashmap.h"
 #include "strbuf.h"
 
@@ -83,23 +83,23 @@ static void t_replace(struct hashmap *map, unsigned int ignore_case)
 	struct test_entry *entry;
 
 	entry = alloc_test_entry("key1", "value1", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 
 	entry = alloc_test_entry(ignore_case ? "Key1" : "key1", "value2",
 				 ignore_case);
 	entry = hashmap_put_entry(map, entry, ent);
-	if (check(entry != NULL))
-		check_str(get_value(entry), "value1");
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(get_value(entry), "value1");
 	free(entry);
 
 	entry = alloc_test_entry("fooBarFrotz", "value3", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 
 	entry = alloc_test_entry(ignore_case ? "FOObarFrotz" : "fooBarFrotz",
 				 "value4", ignore_case);
 	entry = hashmap_put_entry(map, entry, ent);
-	if (check(entry != NULL))
-		check_str(get_value(entry), "value3");
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(get_value(entry), "value3");
 	free(entry);
 }
 
@@ -122,20 +122,18 @@ static void t_get(struct hashmap *map, unsigned int ignore_case)
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1],
 					 ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
 		entry = get_test_entry(map, query[i][0], ignore_case);
-		if (check(entry != NULL))
-			check_str(get_value(entry), query[i][1]);
-		else
-			test_msg("query key: %s", query[i][0]);
+		cl_assert(entry != NULL);
+		cl_assert_equal_s(get_value(entry), query[i][1]);
 	}
 
-	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
+	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
 }
 
 static void t_add(struct hashmap *map, unsigned int ignore_case)
@@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
 
 		hashmap_for_each_entry_from(map, entry, ent)
 		{
-			int ret;
-			if (!check_int((ret = key_val_contains(
-						key_val, seen,
-						ARRAY_SIZE(key_val), entry)),
-				       ==, 0)) {
-				switch (ret) {
-				case 1:
-					test_msg("found entry was not given in the input\n"
-						 "    key: %s\n  value: %s",
-						 entry->key, get_value(entry));
-					break;
-				case 2:
-					test_msg("duplicate entry detected\n"
-						 "    key: %s\n  value: %s",
-						 entry->key, get_value(entry));
-					break;
-				}
-			} else {
-				count++;
-			}
+			int ret = key_val_contains(key_val, seen,
+						   ARRAY_SIZE(key_val), entry);
+			cl_assert(ret == 0);
+			count++;
 		}
-		check_int(count, ==, 2);
+		cl_assert_equal_i(count, 2);
 	}
 
-	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
-		if (!check_int(seen[i], ==, 1))
-			test_msg("following key-val pair was not iterated over:\n"
-				 "    key: %s\n  value: %s",
-				 key_val[i][0], key_val[i][1]);
-	}
+	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
+		cl_assert_equal_i(seen[i], 1);
 
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
-	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
+	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
 }
 
 static void t_remove(struct hashmap *map, unsigned int ignore_case)
@@ -211,24 +189,25 @@ static void t_remove(struct hashmap *map, unsigned int ignore_case)
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	for (size_t i = 0; i < ARRAY_SIZE(remove); i++) {
 		entry = alloc_test_entry(remove[i][0], "", ignore_case);
 		removed = hashmap_remove_entry(map, entry, ent, remove[i][0]);
-		if (check(removed != NULL))
-			check_str(get_value(removed), remove[i][1]);
+		cl_assert(removed != NULL);
+		cl_assert_equal_s(get_value(removed), remove[i][1]);
 		free(entry);
 		free(removed);
 	}
 
 	entry = alloc_test_entry("notInMap", "", ignore_case);
-	check_pointer_eq(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
+	cl_assert_equal_p(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
 	free(entry);
 
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map),
+			  ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
 }
 
 static void t_iterate(struct hashmap *map, unsigned int ignore_case)
@@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
 	{
-		int ret;
-		if (!check_int((ret = key_val_contains(key_val, seen,
-						       ARRAY_SIZE(key_val),
-						       entry)), ==, 0)) {
-			switch (ret) {
-			case 1:
-				test_msg("found entry was not given in the input\n"
-					 "    key: %s\n  value: %s",
-					 entry->key, get_value(entry));
-				break;
-			case 2:
-				test_msg("duplicate entry detected\n"
-					 "    key: %s\n  value: %s",
-					 entry->key, get_value(entry));
-				break;
-			}
-		}
+		int ret = key_val_contains(key_val, seen,
+						ARRAY_SIZE(key_val),
+						entry);
+		cl_assert(ret == 0);
 	}
 
-	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
-		if (!check_int(seen[i], ==, 1))
-			test_msg("following key-val pair was not iterated over:\n"
-				 "    key: %s\n  value: %s",
-				 key_val[i][0], key_val[i][1]);
-	}
+	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
+		cl_assert_equal_i(seen[i], 1);
 
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
 }
 
 static void t_alloc(struct hashmap *map, unsigned int ignore_case)
@@ -284,17 +246,17 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
 		char *key = xstrfmt("key%d", i);
 		char *value = xstrfmt("value%d", i);
 		entry = alloc_test_entry(key, value, ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 		free(key);
 		free(value);
 	}
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, 51);
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), 51);
 
 	entry = alloc_test_entry("key52", "value52", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
-	check_int(map->tablesize, ==, 256);
-	check_int(hashmap_get_size(map), ==, 52);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_i(map->tablesize, 256);
+	cl_assert_equal_i(hashmap_get_size(map), 52);
 
 	for (int i = 1; i <= 12; i++) {
 		char *key = xstrfmt("key%d", i);
@@ -302,27 +264,27 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
 
 		entry = alloc_test_entry(key, "", ignore_case);
 		removed = hashmap_remove_entry(map, entry, ent, key);
-		if (check(removed != NULL))
-			check_str(value, get_value(removed));
+		cl_assert(removed != NULL);
+		cl_assert_equal_s(value, get_value(removed));
 		free(key);
 		free(value);
 		free(entry);
 		free(removed);
 	}
-	check_int(map->tablesize, ==, 256);
-	check_int(hashmap_get_size(map), ==, 40);
+	cl_assert_equal_i(map->tablesize, 256);
+	cl_assert_equal_i(hashmap_get_size(map), 40);
 
 	entry = alloc_test_entry("key40", "", ignore_case);
 	removed = hashmap_remove_entry(map, entry, ent, "key40");
-	if (check(removed != NULL))
-		check_str("value40", get_value(removed));
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, 39);
+	cl_assert(removed != NULL);
+	cl_assert_equal_s("value40", get_value(removed));
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), 39);
 	free(entry);
 	free(removed);
 }
 
-static void t_intern(void)
+void test_hashmap__intern(void)
 {
 	const char *values[] = { "value1", "Value1", "value2", "value2" };
 
@@ -330,32 +292,68 @@ static void t_intern(void)
 		const char *i1 = strintern(values[i]);
 		const char *i2 = strintern(values[i]);
 
-		if (!check(!strcmp(i1, values[i])))
-			test_msg("strintern(%s) returns %s\n", values[i], i1);
-		else if (!check(i1 != values[i]))
-			test_msg("strintern(%s) returns input pointer\n",
-				 values[i]);
-		else if (!check_pointer_eq(i1, i2))
-			test_msg("address('%s') != address('%s'), so strintern('%s') != strintern('%s')",
-				 i1, i2, values[i], values[i]);
-		else
-			check_str(i1, values[i]);
+		cl_assert_equal_s(i1, values[i]);
+		cl_assert(i1 != values[i]);
+		cl_assert_equal_p(i1, i2);
 	}
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_hashmap__replace_case_sensitive(void)
+{
+	setup(t_replace, 0);
+}
+
+void test_hashmap__replace_case_insensitive(void)
+{
+	setup(t_replace, 1);
+}
+
+void test_hashmap__get_case_sensitive(void)
+{
+	setup(t_get, 0);
+}
+
+void test_hashmap__get_case_insensitive(void)
+{
+	setup(t_get, 1);
+}
+
+void test_hashmap__add_case_sensitive(void)
+{
+	setup(t_add, 0);
+}
+
+void test_hashmap__add_case_insensitive(void)
+{
+	setup(t_add, 1);
+}
+
+void test_hashmap__remove_case_sensitive(void)
+{
+	setup(t_remove, 0);
+}
+
+void test_hashmap__remove_case_insensitive(void)
+{
+	setup(t_remove, 1);
+}
+
+void test_hashmap__iterate_case_sensitive(void)
+{
+	setup(t_iterate, 0);
+}
+
+void test_hashmap__iterate_case_insensitive(void)
+{
+	setup(t_iterate, 1);
+}
+
+void test_hashmap__alloc_case_sensitive(void)
+{
+	setup(t_alloc, 0);
+}
+
+void test_hashmap__alloc_case_insensitive(void)
 {
-	TEST(setup(t_replace, 0), "replace works");
-	TEST(setup(t_replace, 1), "replace (case insensitive) works");
-	TEST(setup(t_get, 0), "get works");
-	TEST(setup(t_get, 1), "get (case insensitive) works");
-	TEST(setup(t_add, 0), "add works");
-	TEST(setup(t_add, 1), "add (case insensitive) works");
-	TEST(setup(t_remove, 0), "remove works");
-	TEST(setup(t_remove, 1), "remove (case insensitive) works");
-	TEST(setup(t_iterate, 0), "iterate works");
-	TEST(setup(t_iterate, 1), "iterate (case insensitive) works");
-	TEST(setup(t_alloc, 0), "grow / shrink works");
-	TEST(t_intern(), "string interning works");
-	return test_done();
+	setup(t_alloc, 1);
 }
-- 
2.47.0.86.g15030f9556

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

[Patrick Steinhardt]

On Thu, Jan 30, 2025 at 10:13:31AM +0100, Seyi Kuforiji wrote:
> Adapts hashmap test script to clar framework by using clar assertions

s/Adapts/Adapt as we generally want to use imperative wording in commit
messages, as if we're instructing the code to change.

Also, please note that the code is not a script. Scripts get executed by
an interpreter, but C files are compiled by a compiler before they can
get executed. So you should be talking about "code", not "scripts".

> where necessary. Test functions are created as both standalone and
> inline to test different test cases.

I honestly don't quite know what this second sentence is supposed to say
:)

> diff --git a/t/unit-tests/t-hashmap.c b/t/unit-tests/u-hashmap.c
> similarity index 60%
> rename from t/unit-tests/t-hashmap.c
> rename to t/unit-tests/u-hashmap.c
> index 83b79dff39..6b6d22005a 100644
> --- a/t/unit-tests/t-hashmap.c
> +++ b/t/unit-tests/u-hashmap.c
> @@ -1,4 +1,4 @@
> -#include "test-lib.h"
> +#include "unit-test.h"
>  #include "hashmap.h"
>  #include "strbuf.h"
>  
> @@ -83,23 +83,23 @@ static void t_replace(struct hashmap *map, unsigned int ignore_case)
>  	struct test_entry *entry;
>  
>  	entry = alloc_test_entry("key1", "value1", ignore_case);
> -	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>  
>  	entry = alloc_test_entry(ignore_case ? "Key1" : "key1", "value2",
>  				 ignore_case);
>  	entry = hashmap_put_entry(map, entry, ent);
> -	if (check(entry != NULL))
> -		check_str(get_value(entry), "value1");
> +	cl_assert(entry != NULL);

We usually avoid explicit `!= NULL`, but I guess in this case it's fine
to keep this as-is as you simply keep the preexisting style.

> @@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
>  
>  		hashmap_for_each_entry_from(map, entry, ent)
>  		{
> -			int ret;
> -			if (!check_int((ret = key_val_contains(
> -						key_val, seen,
> -						ARRAY_SIZE(key_val), entry)),
> -				       ==, 0)) {
> -				switch (ret) {
> -				case 1:
> -					test_msg("found entry was not given in the input\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				case 2:
> -					test_msg("duplicate entry detected\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				}
> -			} else {
> -				count++;
> -			}
> +			int ret = key_val_contains(key_val, seen,
> +						   ARRAY_SIZE(key_val), entry);
> +			cl_assert(ret == 0);

This could instead use `cl_assert_equal_i(ret, 0)` so that the error
message mentions what the observed error code is.

> @@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
>  
>  	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
>  		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>  	}
>  
>  	hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
>  	{
> -		int ret;
> -		if (!check_int((ret = key_val_contains(key_val, seen,
> -						       ARRAY_SIZE(key_val),
> -						       entry)), ==, 0)) {
> -			switch (ret) {
> -			case 1:
> -				test_msg("found entry was not given in the input\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			case 2:
> -				test_msg("duplicate entry detected\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			}
> -		}
> +		int ret = key_val_contains(key_val, seen,
> +						ARRAY_SIZE(key_val),
> +						entry);

Indentation is off here.

> @@ -330,32 +292,68 @@ static void t_intern(void)
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_hashmap__replace_case_sensitive(void)
> +{
> +	setup(t_replace, 0);
> +}

I was briefly wondering whether we should use `__initialize()` and
`__teardown()` functions instead of this setup function so that we can
then get rid of `t_replace()` and other test functions. But then I
realized that we want ot have these separate functions anyway so that we
can easily test with case-sensitivity enabled and disabled, so this is
probably okay.

Patrick

[PATCH 2/4] t/unit-tests: adapt example decorate test to clar framework

[Seyi Kuforiji]

Adapts example decorate test script to clar framework by using clar
assertions where necessary. Test functions are created as standalone to
test different test cases.

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-example-decorate.c | 74 ------------------------------
 t/unit-tests/u-example-decorate.c | 76 +++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 76 deletions(-)
 delete mode 100644 t/unit-tests/t-example-decorate.c
 create mode 100644 t/unit-tests/u-example-decorate.c

diff --git a/Makefile b/Makefile
index 2d9dad119a..732d765f1c 100644
--- a/Makefile
+++ b/Makefile
@@ -1338,6 +1338,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
 THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
 CLAR_TEST_SUITES += u-ctype
+CLAR_TEST_SUITES += u-example-decorate
 CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
@@ -1349,7 +1350,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-example-decorate
 UNIT_TEST_PROGRAMS += t-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
diff --git a/t/meson.build b/t/meson.build
index af597f9804..c7e08eca6f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,5 +1,6 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
+  'unit-tests/u-example-decorate.c',
   'unit-tests/u-hash.c',
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
@@ -45,7 +46,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-example-decorate.c',
   'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
diff --git a/t/unit-tests/t-example-decorate.c b/t/unit-tests/t-example-decorate.c
deleted file mode 100644
index bfc776e223..0000000000
--- a/t/unit-tests/t-example-decorate.c
+++ /dev/null
@@ -1,74 +0,0 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
-#include "test-lib.h"
-#include "object.h"
-#include "decorate.h"
-#include "repository.h"
-
-struct test_vars {
-	struct object *one, *two, *three;
-	struct decoration n;
-	int decoration_a, decoration_b;
-};
-
-static void t_add(struct test_vars *vars)
-{
-	void *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);
-
-	check(ret == NULL);
-	ret = add_decoration(&vars->n, vars->two, NULL);
-	check(ret == NULL);
-}
-
-static void t_readd(struct test_vars *vars)
-{
-	void *ret = add_decoration(&vars->n, vars->one, NULL);
-
-	check(ret == &vars->decoration_a);
-	ret = add_decoration(&vars->n, vars->two, &vars->decoration_b);
-	check(ret == NULL);
-}
-
-static void t_lookup(struct test_vars *vars)
-{
-	void *ret = lookup_decoration(&vars->n, vars->one);
-
-	check(ret == NULL);
-	ret = lookup_decoration(&vars->n, vars->two);
-	check(ret == &vars->decoration_b);
-	ret = lookup_decoration(&vars->n, vars->three);
-	check(ret == NULL);
-}
-
-static void t_loop(struct test_vars *vars)
-{
-	int objects_noticed = 0;
-
-	for (size_t i = 0; i < vars->n.size; i++) {
-		if (vars->n.entries[i].base)
-			objects_noticed++;
-	}
-	check_int(objects_noticed, ==, 2);
-}
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
-	struct test_vars vars = { 0 };
-
-	vars.one = lookup_unknown_object(the_repository, &one_oid);
-	vars.two = lookup_unknown_object(the_repository, &two_oid);
-	vars.three = lookup_unknown_object(the_repository, &three_oid);
-
-	TEST(t_add(&vars),
-	     "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.");
-	TEST(t_readd(&vars),
-	     "When re-adding an already existing object, the old decoration is returned.");
-	TEST(t_lookup(&vars),
-	     "Lookup returns the added declarations, or NULL if the object was never added.");
-	TEST(t_loop(&vars), "The user can also loop through all entries.");
-
-	clear_decoration(&vars.n, NULL);
-
-	return test_done();
-}
diff --git a/t/unit-tests/u-example-decorate.c b/t/unit-tests/u-example-decorate.c
new file mode 100644
index 0000000000..3a457d41fc
--- /dev/null
+++ b/t/unit-tests/u-example-decorate.c
@@ -0,0 +1,76 @@
+#define USE_THE_REPOSITORY_VARIABLE
+
+#include "unit-test.h"
+#include "object.h"
+#include "decorate.h"
+#include "repository.h"
+
+struct test_vars {
+	struct object *one, *two, *three;
+	struct decoration n;
+	int decoration_a, decoration_b;
+};
+
+static struct test_vars vars;
+
+void test_example_decorate__add(void)
+{
+	void *ret = add_decoration(&vars.n, vars.one, &vars.decoration_a);
+	cl_assert(ret == NULL);
+	ret = add_decoration(&vars.n, vars.two, NULL);
+	cl_assert(ret == NULL);
+}
+
+void test_example_decorate__readd(void)
+{
+	void *ret;
+
+	cl_assert(add_decoration(&vars.n, vars.one, &vars.decoration_a) == NULL);
+	cl_assert(add_decoration(&vars.n, vars.two, NULL) == NULL);
+
+	ret = add_decoration(&vars.n, vars.one, NULL);
+	cl_assert(ret == &vars.decoration_a);
+	ret = add_decoration(&vars.n, vars.two, &vars.decoration_b);
+	cl_assert(ret == NULL);
+}
+
+void test_example_decorate__lookup(void)
+{
+	void *ret;
+
+	add_decoration(&vars.n, vars.two, &vars.decoration_b);
+	add_decoration(&vars.n, vars.one, NULL);
+
+	ret = lookup_decoration(&vars.n, vars.two);
+	cl_assert(ret == &vars.decoration_b);
+	ret = lookup_decoration(&vars.n, vars.one);
+	cl_assert(ret == NULL);
+}
+
+void test_example_decorate__loop(void)
+{
+	int objects_noticed = 0;
+
+	add_decoration(&vars.n, vars.one, &vars.decoration_a);
+	add_decoration(&vars.n, vars.two, &vars.decoration_b);
+
+	for (size_t i = 0; i < vars.n.size; i++) {
+		if (vars.n.entries[i].base)
+			objects_noticed++;
+	}
+	cl_assert_equal_i(objects_noticed, 2);
+}
+
+void test_example_decorate__initialize(void)
+{
+	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
+
+	vars.one = lookup_unknown_object(the_repository, &one_oid);
+	vars.two = lookup_unknown_object(the_repository, &two_oid);
+	vars.three = lookup_unknown_object(the_repository, &three_oid);
+}
+
+void test_example_decorate__cleanup(void)
+{
+	clear_decoration(&vars.n, NULL);
+}
-- 
2.47.0.86.g15030f9556

Re: [PATCH 2/4] t/unit-tests: adapt example decorate test to clar framework

[Patrick Steinhardt]

On Thu, Jan 30, 2025 at 10:13:32AM +0100, Seyi Kuforiji wrote:
> Adapts example decorate test script to clar framework by using clar
> assertions where necessary. Test functions are created as standalone to
> test different test cases.

Again, I don't think that the second sentence doesn't add much
information to the commit message and can probably just be dropped. It
would be more interesting to talk about any design decisions that
weren't immediately obvious, as these may also make the reviewer wonder.

One thing that would be worth to explain here are the `initialize()` and
`cleanup()` functions. Readers not familiar with the clar framework may
not know that those get picked up by the framework automatically and
that they are executed before and after every single test.

> 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-example-decorate.c | 74 ------------------------------
>  t/unit-tests/u-example-decorate.c | 76 +++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 76 deletions(-)
>  delete mode 100644 t/unit-tests/t-example-decorate.c
>  create mode 100644 t/unit-tests/u-example-decorate.c
> 
> diff --git a/Makefile b/Makefile
> index 2d9dad119a..732d765f1c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1338,6 +1338,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
>  THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
>  
>  CLAR_TEST_SUITES += u-ctype
> +CLAR_TEST_SUITES += u-example-decorate
>  CLAR_TEST_SUITES += u-hash
>  CLAR_TEST_SUITES += u-hashmap
>  CLAR_TEST_SUITES += u-mem-pool
> @@ -1349,7 +1350,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-example-decorate
>  UNIT_TEST_PROGRAMS += t-oid-array
>  UNIT_TEST_PROGRAMS += t-oidmap
>  UNIT_TEST_PROGRAMS += t-oidtree
> diff --git a/t/meson.build b/t/meson.build
> index af597f9804..c7e08eca6f 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1,5 +1,6 @@
>  clar_test_suites = [
>    'unit-tests/u-ctype.c',
> +  'unit-tests/u-example-decorate.c',
>    'unit-tests/u-hash.c',
>    'unit-tests/u-hashmap.c',
>    'unit-tests/u-mem-pool.c',
> @@ -45,7 +46,6 @@ clar_unit_tests = executable('unit-tests',
>  test('unit-tests', clar_unit_tests)
>  
>  unit_test_programs = [
> -  'unit-tests/t-example-decorate.c',
>    'unit-tests/t-oid-array.c',
>    'unit-tests/t-oidmap.c',
>    'unit-tests/t-oidtree.c',
> diff --git a/t/unit-tests/t-example-decorate.c b/t/unit-tests/t-example-decorate.c
> deleted file mode 100644
> index bfc776e223..0000000000
> --- a/t/unit-tests/t-example-decorate.c
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> -
> -#include "test-lib.h"
> -#include "object.h"
> -#include "decorate.h"
> -#include "repository.h"
> -
> -struct test_vars {
> -	struct object *one, *two, *three;
> -	struct decoration n;
> -	int decoration_a, decoration_b;
> -};
> -
> -static void t_add(struct test_vars *vars)
> -{
> -	void *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);
> -
> -	check(ret == NULL);
> -	ret = add_decoration(&vars->n, vars->two, NULL);
> -	check(ret == NULL);
> -}
> -
> -static void t_readd(struct test_vars *vars)
> -{
> -	void *ret = add_decoration(&vars->n, vars->one, NULL);
> -
> -	check(ret == &vars->decoration_a);
> -	ret = add_decoration(&vars->n, vars->two, &vars->decoration_b);
> -	check(ret == NULL);
> -}
> -
> -static void t_lookup(struct test_vars *vars)
> -{
> -	void *ret = lookup_decoration(&vars->n, vars->one);
> -
> -	check(ret == NULL);
> -	ret = lookup_decoration(&vars->n, vars->two);
> -	check(ret == &vars->decoration_b);
> -	ret = lookup_decoration(&vars->n, vars->three);
> -	check(ret == NULL);
> -}
> -
> -static void t_loop(struct test_vars *vars)
> -{
> -	int objects_noticed = 0;
> -
> -	for (size_t i = 0; i < vars->n.size; i++) {
> -		if (vars->n.entries[i].base)
> -			objects_noticed++;
> -	}
> -	check_int(objects_noticed, ==, 2);
> -}
> -
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> -{
> -	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
> -	struct test_vars vars = { 0 };
> -
> -	vars.one = lookup_unknown_object(the_repository, &one_oid);
> -	vars.two = lookup_unknown_object(the_repository, &two_oid);
> -	vars.three = lookup_unknown_object(the_repository, &three_oid);
> -
> -	TEST(t_add(&vars),
> -	     "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.");
> -	TEST(t_readd(&vars),
> -	     "When re-adding an already existing object, the old decoration is returned.");
> -	TEST(t_lookup(&vars),
> -	     "Lookup returns the added declarations, or NULL if the object was never added.");
> -	TEST(t_loop(&vars), "The user can also loop through all entries.");
> -
> -	clear_decoration(&vars.n, NULL);
> -
> -	return test_done();
> -}
> diff --git a/t/unit-tests/u-example-decorate.c b/t/unit-tests/u-example-decorate.c
> new file mode 100644
> index 0000000000..3a457d41fc
> --- /dev/null
> +++ b/t/unit-tests/u-example-decorate.c
> @@ -0,0 +1,76 @@
> +#define USE_THE_REPOSITORY_VARIABLE
> +
> +#include "unit-test.h"
> +#include "object.h"
> +#include "decorate.h"
> +#include "repository.h"
> +
> +struct test_vars {
> +	struct object *one, *two, *three;
> +	struct decoration n;
> +	int decoration_a, decoration_b;
> +};
> +
> +static struct test_vars vars;
> +
> +void test_example_decorate__add(void)
> +{
> +	void *ret = add_decoration(&vars.n, vars.one, &vars.decoration_a);
> +	cl_assert(ret == NULL);
> +	ret = add_decoration(&vars.n, vars.two, NULL);
> +	cl_assert(ret == NULL);
> +}

We could avoid the separate variable here, e.g. like this:

void test_example_decorate__add(void)
{
	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
}

The same is true for some of the other test, too.

The test itself could be a bit more thorough in verifying what has been
done, but that is not a new issue that your conversion introduces as the
old testcase was somewhat light on that, too. So this is nothing that I
think you need to change.

> +void test_example_decorate__readd(void)
> +{
> +	void *ret;
> +
> +	cl_assert(add_decoration(&vars.n, vars.one, &vars.decoration_a) == NULL);
> +	cl_assert(add_decoration(&vars.n, vars.two, NULL) == NULL);

Okay. This wasn't present before because tests were actually dependent
on the data that previous tests wrote. Now that we decouple the tests
from one another we have to make sure to recreate the expected setup.
Which I think is a good thing, as it makes tests fully self-contained
and the setup easier to understand.

I guess this is another thing that you should be explaining in the
commit message, as it is not obvious at all.

> +	ret = add_decoration(&vars.n, vars.one, NULL);
> +	cl_assert(ret == &vars.decoration_a);
> +	ret = add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +	cl_assert(ret == NULL);
> +}
> +
> +void test_example_decorate__lookup(void)
> +{
> +	void *ret;
> +
> +	add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +	add_decoration(&vars.n, vars.one, NULL);
> +
> +	ret = lookup_decoration(&vars.n, vars.two);
> +	cl_assert(ret == &vars.decoration_b);
> +	ret = lookup_decoration(&vars.n, vars.one);
> +	cl_assert(ret == NULL);
> +}
> +
> +void test_example_decorate__loop(void)
> +{
> +	int objects_noticed = 0;
> +
> +	add_decoration(&vars.n, vars.one, &vars.decoration_a);
> +	add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +
> +	for (size_t i = 0; i < vars.n.size; i++) {
> +		if (vars.n.entries[i].base)
> +			objects_noticed++;
> +	}

Nit: we can get rid of the curly braces here.

> +	cl_assert_equal_i(objects_noticed, 2);
> +}
> +
> +void test_example_decorate__initialize(void)
> +{
> +	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
> +
> +	vars.one = lookup_unknown_object(the_repository, &one_oid);
> +	vars.two = lookup_unknown_object(the_repository, &two_oid);
> +	vars.three = lookup_unknown_object(the_repository, &three_oid);
> +}
> +
> +void test_example_decorate__cleanup(void)
> +{
> +	clear_decoration(&vars.n, NULL);
> +}

Let's move both `initialize()` and `cleanup()` to the top so that it
becomes easy to see that all tests use this commen setup.

Patrick

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

[Seyi Kuforiji]

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

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-strbuf.c | 122 ----------------------------------------
 t/unit-tests/u-strbuf.c | 121 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 124 deletions(-)
 delete mode 100644 t/unit-tests/t-strbuf.c
 create mode 100644 t/unit-tests/u-strbuf.c

diff --git a/Makefile b/Makefile
index 732d765f1c..358193597f 100644
--- a/Makefile
+++ b/Makefile
@@ -1344,6 +1344,7 @@ CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
+CLAR_TEST_SUITES += u-strbuf
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
@@ -1361,7 +1362,6 @@ UNIT_TEST_PROGRAMS += t-reftable-reader
 UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-reftable-stack
-UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
diff --git a/t/meson.build b/t/meson.build
index c7e08eca6f..6cb72842b1 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-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
+  'unit-tests/u-strbuf.c',
   'unit-tests/u-strvec.c',
 ]
 
@@ -57,7 +58,6 @@ unit_test_programs = [
   'unit-tests/t-reftable-readwrite.c',
   'unit-tests/t-reftable-record.c',
   'unit-tests/t-reftable-stack.c',
-  'unit-tests/t-strbuf.c',
   'unit-tests/t-strcmp-offset.c',
   'unit-tests/t-trailer.c',
   'unit-tests/t-urlmatch-normalization.c',
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
deleted file mode 100644
index 3f4044d697..0000000000
--- a/t/unit-tests/t-strbuf.c
+++ /dev/null
@@ -1,122 +0,0 @@
-#include "test-lib.h"
-#include "strbuf.h"
-
-/* wrapper that supplies tests with an empty, initialized strbuf */
-static void setup(void (*f)(struct strbuf*, const void*),
-		  const void *data)
-{
-	struct strbuf buf = STRBUF_INIT;
-
-	f(&buf, data);
-	strbuf_release(&buf);
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
-}
-
-/* wrapper that supplies tests with a populated, initialized strbuf */
-static void setup_populated(void (*f)(struct strbuf*, const void*),
-			    const char *init_str, const void *data)
-{
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addstr(&buf, init_str);
-	check_uint(buf.len, ==, strlen(init_str));
-	f(&buf, data);
-	strbuf_release(&buf);
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
-}
-
-static int assert_sane_strbuf(struct strbuf *buf)
-{
-	/* Initialized strbufs should always have a non-NULL buffer */
-	if (!check(!!buf->buf))
-		return 0;
-	/* Buffers should always be NUL-terminated */
-	if (!check_char(buf->buf[buf->len], ==, '\0'))
-		return 0;
-	/*
-	 * Freshly-initialized strbufs may not have a dynamically allocated
-	 * buffer
-	 */
-	if (buf->len == 0 && buf->alloc == 0)
-		return 1;
-	/* alloc must be at least one byte larger than len */
-	return check_uint(buf->len, <, buf->alloc);
-}
-
-static void t_static_init(void)
-{
-	struct strbuf buf = STRBUF_INIT;
-
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
-	check_char(buf.buf[0], ==, '\0');
-}
-
-static void t_dynamic_init(void)
-{
-	struct strbuf buf;
-
-	strbuf_init(&buf, 1024);
-	check(assert_sane_strbuf(&buf));
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, >=, 1024);
-	check_char(buf.buf[0], ==, '\0');
-	strbuf_release(&buf);
-}
-
-static void t_addch(struct strbuf *buf, const void *data)
-{
-	const char *p_ch = data;
-	const char ch = *p_ch;
-	size_t orig_alloc = buf->alloc;
-	size_t orig_len = buf->len;
-
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	strbuf_addch(buf, ch);
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	if (!(check_uint(buf->len, ==, orig_len + 1) &&
-	      check_uint(buf->alloc, >=, orig_alloc)))
-		return; /* avoid de-referencing buf->buf */
-	check_char(buf->buf[buf->len - 1], ==, ch);
-	check_char(buf->buf[buf->len], ==, '\0');
-}
-
-static void t_addstr(struct strbuf *buf, const void *data)
-{
-	const char *text = data;
-	size_t len = strlen(text);
-	size_t orig_alloc = buf->alloc;
-	size_t orig_len = buf->len;
-
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	strbuf_addstr(buf, text);
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	if (!(check_uint(buf->len, ==, orig_len + len) &&
-	      check_uint(buf->alloc, >=, orig_alloc) &&
-	      check_uint(buf->alloc, >, orig_len + len) &&
-	      check_char(buf->buf[orig_len + len], ==, '\0')))
-	    return;
-	check_str(buf->buf + orig_len, text);
-}
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	if (!TEST(t_static_init(), "static initialization works"))
-		test_skip_all("STRBUF_INIT is broken");
-	TEST(t_dynamic_init(), "dynamic initialization works");
-	TEST(setup(t_addch, "a"), "strbuf_addch adds char");
-	TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
-	TEST(setup_populated(t_addch, "initial value", "a"),
-	     "strbuf_addch appends to initial value");
-	TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
-	TEST(setup_populated(t_addstr, "initial value", "hello there"),
-	     "strbuf_addstr appends string to initial value");
-
-	return test_done();
-}
diff --git a/t/unit-tests/u-strbuf.c b/t/unit-tests/u-strbuf.c
new file mode 100644
index 0000000000..fec3c768d2
--- /dev/null
+++ b/t/unit-tests/u-strbuf.c
@@ -0,0 +1,121 @@
+#include "unit-test.h"
+#include "strbuf.h"
+
+/* wrapper that supplies tests with an empty, initialized strbuf */
+static void setup(void (*f)(struct strbuf*, const void*),
+		  const void *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	f(&buf, data);
+	strbuf_release(&buf);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
+}
+
+/* wrapper that supplies tests with a populated, initialized strbuf */
+static void setup_populated(void (*f)(struct strbuf*, const void*),
+			    const char *init_str, const void *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addstr(&buf, init_str);
+	cl_assert_equal_i(buf.len, strlen(init_str));
+	f(&buf, data);
+	strbuf_release(&buf);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
+}
+
+static void assert_sane_strbuf(struct strbuf *buf)
+{
+	/* Initialized strbufs should always have a non-NULL buffer */
+	cl_assert(buf->buf != NULL);
+	/* Buffers should always be NUL-terminated */
+	cl_assert(buf->buf[buf->len] == '\0');
+	/*
+	 * Freshly-initialized strbufs may not have a dynamically allocated
+	 * buffer
+	 */
+	if (buf->len == 0 && buf->alloc == 0)
+		return;
+	/* alloc must be at least one byte larger than len */
+	cl_assert(buf->len < buf->alloc);
+}
+
+void test_strbuf__static_init(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
+	cl_assert(buf.buf[0] == '\0');
+}
+
+void test_strbuf__dynamic_init(void)
+{
+	struct strbuf buf;
+
+	strbuf_init(&buf, 1024);
+	assert_sane_strbuf(&buf);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert(buf.alloc >= 1024);
+	cl_assert(buf.buf[0] == '\0');
+	strbuf_release(&buf);
+}
+
+static void t_addch(struct strbuf *buf, const void *data)
+{
+	const char *p_ch = data;
+	const char ch = *p_ch;
+	size_t orig_alloc = buf->alloc;
+	size_t orig_len = buf->len;
+
+	assert_sane_strbuf(buf);
+	strbuf_addch(buf, ch);
+	assert_sane_strbuf(buf);
+	cl_assert_equal_i(buf->len, orig_len + 1);
+	cl_assert(buf->alloc >= orig_alloc);
+	cl_assert(buf->buf[buf->len] == '\0');
+}
+
+static void t_addstr(struct strbuf *buf, const void *data)
+{
+	const char *text = data;
+	size_t len = strlen(text);
+	size_t orig_alloc = buf->alloc;
+	size_t orig_len = buf->len;
+
+	assert_sane_strbuf(buf);
+	strbuf_addstr(buf, text);
+	assert_sane_strbuf(buf);
+	cl_assert_equal_i(buf->len, orig_len + len);
+	cl_assert(buf->alloc >= orig_alloc);
+	cl_assert(buf->buf[buf->len] == '\0');
+	cl_assert_equal_s(buf->buf + orig_len, text);
+}
+
+void test_strbuf__add_single_char(void)
+{
+	setup(t_addch, "a");
+}
+
+void test_strbuf__add_empty_char(void)
+{
+	setup(t_addch, "");
+}
+
+void test_strbuf__add_multi_char(void)
+{
+	setup_populated(t_addch, "initial value", "a");
+}
+
+void test_strbuf__add_single_str(void)
+{
+	setup(t_addstr, "hello there");
+}
+
+void test_strbuf__add_multi_str(void)
+{
+	setup_populated(t_addstr, "initial value", "hello there");
+}
-- 
2.47.0.86.g15030f9556

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

[Patrick Steinhardt]

On Thu, Jan 30, 2025 at 10:13:33AM +0100, Seyi Kuforiji wrote:
> Adapt strbuf test script to clar framework by using clar assertions
> where necessary. Test functions are created as standalone to test
> different test cases.

Same comment here regarding the second sentence.

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

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

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

I think this condition can be simplified a bit:

        /*
         * In case the buffer contains anything, `alloc` must alloc must
         * be at least one byte larger than `len`.
         */
        if (buf->len)
            cl_assert(buf->len < buf->alloc);

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

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

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

Similarly, we could name this "append_str".

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

Patrick

[PATCH 4/4] t/unit-tests: convert strcmp-offset test to clar framework

[Seyi Kuforiji]

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

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-strcmp-offset.c | 35 --------------------------
 t/unit-tests/u-strcmp-offset.c | 45 ++++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+), 37 deletions(-)
 delete mode 100644 t/unit-tests/t-strcmp-offset.c
 create mode 100644 t/unit-tests/u-strcmp-offset.c

diff --git a/Makefile b/Makefile
index 358193597f..76b5de4fdd 100644
--- a/Makefile
+++ b/Makefile
@@ -1345,6 +1345,7 @@ CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
+CLAR_TEST_SUITES += u-strcmp-offset
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
@@ -1362,7 +1363,6 @@ UNIT_TEST_PROGRAMS += t-reftable-reader
 UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-reftable-stack
-UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
diff --git a/t/meson.build b/t/meson.build
index 6cb72842b1..3935782bbb 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -7,6 +7,7 @@ clar_test_suites = [
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
+  'unit-tests/u-strcmp-offset.c',
   'unit-tests/u-strvec.c',
 ]
 
@@ -58,7 +59,6 @@ unit_test_programs = [
   'unit-tests/t-reftable-readwrite.c',
   'unit-tests/t-reftable-record.c',
   'unit-tests/t-reftable-stack.c',
-  'unit-tests/t-strcmp-offset.c',
   'unit-tests/t-trailer.c',
   'unit-tests/t-urlmatch-normalization.c',
 ]
diff --git a/t/unit-tests/t-strcmp-offset.c b/t/unit-tests/t-strcmp-offset.c
deleted file mode 100644
index 6880f21161..0000000000
--- a/t/unit-tests/t-strcmp-offset.c
+++ /dev/null
@@ -1,35 +0,0 @@
-#include "test-lib.h"
-#include "read-cache-ll.h"
-
-static void check_strcmp_offset(const char *string1, const char *string2,
-				int expect_result, uintmax_t expect_offset)
-{
-	size_t offset;
-	int result = strcmp_offset(string1, string2, &offset);
-
-	/*
-	 * Because different CRTs behave differently, only rely on signs of the
-	 * result values.
-	 */
-	result = (result < 0 ? -1 :
-			result > 0 ? 1 :
-			0);
-
-	check_int(result, ==, expect_result);
-	check_uint((uintmax_t)offset, ==, expect_offset);
-}
-
-#define TEST_STRCMP_OFFSET(string1, string2, expect_result, expect_offset) \
-	TEST(check_strcmp_offset(string1, string2, expect_result,          \
-				 expect_offset),                           \
-	     "strcmp_offset(%s, %s) works", #string1, #string2)
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_STRCMP_OFFSET("abc", "abc", 0, 3);
-	TEST_STRCMP_OFFSET("abc", "def", -1, 0);
-	TEST_STRCMP_OFFSET("abc", "abz", -1, 2);
-	TEST_STRCMP_OFFSET("abc", "abcdef", -1, 3);
-
-	return test_done();
-}
diff --git a/t/unit-tests/u-strcmp-offset.c b/t/unit-tests/u-strcmp-offset.c
new file mode 100644
index 0000000000..7e8e9acf3c
--- /dev/null
+++ b/t/unit-tests/u-strcmp-offset.c
@@ -0,0 +1,45 @@
+#include "unit-test.h"
+#include "read-cache-ll.h"
+
+static void check_strcmp_offset(const char *string1, const char *string2,
+				int expect_result, uintmax_t expect_offset)
+{
+	size_t offset;
+	int result = strcmp_offset(string1, string2, &offset);
+
+	/*
+	 * Because different CRTs behave differently, only rely on signs of the
+	 * result values.
+	 */
+	result = (result < 0 ? -1 :
+			result > 0 ? 1 :
+			0);
+
+	cl_assert_equal_i(result, expect_result);
+	cl_assert_equal_i((uintmax_t)offset, expect_offset);
+}
+
+void test_strcmp_offset__empty(void)
+{
+	check_strcmp_offset("", "", 0, 0);
+}
+
+void test_strcmp_offset__equal(void)
+{
+	check_strcmp_offset("abc", "abc", 0, 3);
+}
+
+void test_strcmp_offset__different(void)
+{
+	check_strcmp_offset("abc", "def", -1, 0);
+}
+
+void test_strcmp_offset__mismatch(void)
+{
+	check_strcmp_offset("abc", "abz", -1, 2);
+}
+
+void test_strcmp_offset__different_length(void)
+{
+	check_strcmp_offset("abc", "abcdef", -1, 3);
+}
-- 
2.47.0.86.g15030f9556

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

[Patrick Steinhardt]

On Thu, Jan 30, 2025 at 10:13:34AM +0100, Seyi Kuforiji wrote:
> Adapt strcmp-offset test script to clar framework by using clar
> assertions where necessary. Test functions are created as standalone to
> test different test cases.

Same comment here regarding the message.

> diff --git a/t/unit-tests/u-strcmp-offset.c b/t/unit-tests/u-strcmp-offset.c
> new file mode 100644
> index 0000000000..7e8e9acf3c
> --- /dev/null
> +++ b/t/unit-tests/u-strcmp-offset.c
> @@ -0,0 +1,45 @@
> +#include "unit-test.h"
> +#include "read-cache-ll.h"
> +
> +static void check_strcmp_offset(const char *string1, const char *string2,
> +				int expect_result, uintmax_t expect_offset)
> +{
> +	size_t offset;
> +	int result = strcmp_offset(string1, string2, &offset);
> +
> +	/*
> +	 * Because different CRTs behave differently, only rely on signs of the
> +	 * result values.
> +	 */
> +	result = (result < 0 ? -1 :
> +			result > 0 ? 1 :
> +			0);
> +
> +	cl_assert_equal_i(result, expect_result);
> +	cl_assert_equal_i((uintmax_t)offset, expect_offset);
> +}
> +
> +void test_strcmp_offset__empty(void)
> +{
> +	check_strcmp_offset("", "", 0, 0);

This test didn't exist in the preimage, right? I don't mind adding it,
but it is somewhat surprising as the commit message does not mention
this at all.

Patrick

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

[Patrick Steinhardt]

On Thu, Jan 30, 2025 at 10:13:30AM +0100, Seyi Kuforiji wrote:
> Hello,
> 
> This small patch series transitions the 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 for your patches! I've got a couple of comments, but overall this
looks mostly good to me.

Patrick

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

[Seyi Kuforiji]

Hello,

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

Changes in v2:
- small fixes to the commit messages and how they read
- some small code fix up and refactoring

Thanks
Seyi

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

Seyi Kuforiji (4):
  t/unit-tests: convert hashmap test to use clar test framework
  t/unit-tests: adapt example decorate test to use clar test framework
  t/unit-tests: convert strbuf test to use clar test framework
  t/unit-tests: convert strcmp-offset test to use clar test framework

 Makefile                                      |   8 +-
 t/meson.build                                 |   8 +-
 ...xample-decorate.c => u-example-decorate.c} |  76 +++---
 t/unit-tests/{t-hashmap.c => u-hashmap.c}     | 226 +++++++++---------
 t/unit-tests/{t-strbuf.c => u-strbuf.c}       | 115 +++++----
 .../{t-strcmp-offset.c => u-strcmp-offset.c}  |  36 ++-
 6 files changed, 232 insertions(+), 237 deletions(-)
 rename t/unit-tests/{t-example-decorate.c => u-example-decorate.c} (30%)
 rename t/unit-tests/{t-hashmap.c => u-hashmap.c} (60%)
 rename t/unit-tests/{t-strbuf.c => u-strbuf.c} (35%)
 rename t/unit-tests/{t-strcmp-offset.c => u-strcmp-offset.c} (39%)

Range-diff against v1:
1:  90accb2f75 ! 1:  19697be26b t/unit-tests: convert hashmap test to use clar test framework
    @@ Commit message
         t/unit-tests: convert hashmap test to use clar test framework
     
         Adapts hashmap test script to clar framework by using clar assertions
    -    where necessary. Test functions are created as both standalone and
    -    inline to test different test cases.
    +    where necessary.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
    @@ t/unit-tests/u-hashmap.c: static void t_add(struct hashmap *map, unsigned int ig
     -			}
     +			int ret = key_val_contains(key_val, seen,
     +						   ARRAY_SIZE(key_val), entry);
    -+			cl_assert(ret == 0);
    ++			cl_assert_equal_i(ret, 0);
     +			count++;
      		}
     -		check_int(count, ==, 2);
    @@ t/unit-tests/u-hashmap.c: static void t_iterate(struct hashmap *map, unsigned in
     -			}
     -		}
     +		int ret = key_val_contains(key_val, seen,
    -+						ARRAY_SIZE(key_val),
    -+						entry);
    ++					   ARRAY_SIZE(key_val),
    ++					   entry);
     +		cl_assert(ret == 0);
      	}
      
2:  13a407d504 ! 2:  1d8f8974a5 t/unit-tests: adapt example decorate test to use clar test framework
    @@ Metadata
      ## Commit message ##
         t/unit-tests: adapt example decorate test to use clar test framework
     
    -    Adapts example decorate test script to clar framework by using clar
    -    assertions where necessary. Test functions are created as standalone to
    -    test different test cases.
    +    Introduce `test_example_decorate__initialize()` to explicitly set up
    +    object IDs and retrieve corresponding objects before tests run. This
    +    ensures a consistent and predictable test state without relying on data
    +    from previous tests.
    +
    +    Add `test_example_decorate__cleanup()` to clear decorations after each
    +    test, preventing interference between tests and ensuring each runs in
    +    isolation.
    +
    +    Adapt example decorate test script to clar framework by using clar
    +    assertions where necessary. Previously, tests relied on data written by
    +    earlier tests, leading to unintended dependencies between them. This
    +    explicitly initializes the necessary state within
    +    `test_example_decorate__readd`, ensuring it does not depend on prior
    +    test executions.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
    @@ t/unit-tests/u-example-decorate.c (new)
     +
     +static struct test_vars vars;
     +
    -+void test_example_decorate__add(void)
    ++void test_example_decorate__initialize(void)
     +{
    -+	void *ret = add_decoration(&vars.n, vars.one, &vars.decoration_a);
    -+	cl_assert(ret == NULL);
    -+	ret = add_decoration(&vars.n, vars.two, NULL);
    -+	cl_assert(ret == NULL);
    ++	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
    ++
    ++	vars.one = lookup_unknown_object(the_repository, &one_oid);
    ++	vars.two = lookup_unknown_object(the_repository, &two_oid);
    ++	vars.three = lookup_unknown_object(the_repository, &three_oid);
     +}
     +
    -+void test_example_decorate__readd(void)
    ++void test_example_decorate__cleanup(void)
     +{
    -+	void *ret;
    ++	clear_decoration(&vars.n, NULL);
    ++}
     +
    -+	cl_assert(add_decoration(&vars.n, vars.one, &vars.decoration_a) == NULL);
    -+	cl_assert(add_decoration(&vars.n, vars.two, NULL) == NULL);
    ++void test_example_decorate__add(void)
    ++{
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
    ++}
     +
    -+	ret = add_decoration(&vars.n, vars.one, NULL);
    -+	cl_assert(ret == &vars.decoration_a);
    -+	ret = add_decoration(&vars.n, vars.two, &vars.decoration_b);
    -+	cl_assert(ret == NULL);
    ++void test_example_decorate__readd(void)
    ++{
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.one, NULL), &vars.decoration_a);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
     +}
     +
     +void test_example_decorate__lookup(void)
     +{
    -+	void *ret;
    -+
    -+	add_decoration(&vars.n, vars.two, &vars.decoration_b);
    -+	add_decoration(&vars.n, vars.one, NULL);
    -+
    -+	ret = lookup_decoration(&vars.n, vars.two);
    -+	cl_assert(ret == &vars.decoration_b);
    -+	ret = lookup_decoration(&vars.n, vars.one);
    -+	cl_assert(ret == NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.one, NULL), NULL);
    ++	cl_assert_equal_p(lookup_decoration(&vars.n, vars.two), &vars.decoration_b);
    ++	cl_assert_equal_p(lookup_decoration(&vars.n, vars.one), NULL);
     +}
     +
     +void test_example_decorate__loop(void)
     +{
     +	int objects_noticed = 0;
     +
    -+	add_decoration(&vars.n, vars.one, &vars.decoration_a);
    -+	add_decoration(&vars.n, vars.two, &vars.decoration_b);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
    ++	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
     +
    -+	for (size_t i = 0; i < vars.n.size; i++) {
    ++	for (size_t i = 0; i < vars.n.size; i++)
     +		if (vars.n.entries[i].base)
     +			objects_noticed++;
    -+	}
    -+	cl_assert_equal_i(objects_noticed, 2);
    -+}
     +
    -+void test_example_decorate__initialize(void)
    -+{
    -+	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
    -+
    -+	vars.one = lookup_unknown_object(the_repository, &one_oid);
    -+	vars.two = lookup_unknown_object(the_repository, &two_oid);
    -+	vars.three = lookup_unknown_object(the_repository, &three_oid);
    -+}
    -+
    -+void test_example_decorate__cleanup(void)
    -+{
    -+	clear_decoration(&vars.n, NULL);
    ++	cl_assert_equal_i(objects_noticed, 2);
     +}
3:  08ade6b5cf ! 3:  e88ab7ab5f t/unit-tests: convert strbuf test to use clar test framework
    @@ Commit message
         t/unit-tests: convert strbuf test to use clar test framework
     
         Adapt strbuf test script to clar framework by using clar assertions
    -    where necessary. Test functions are created as standalone to test
    -    different test cases.
    +    where necessary.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
    @@ t/unit-tests/u-strbuf.c (new)
     +	/* Buffers should always be NUL-terminated */
     +	cl_assert(buf->buf[buf->len] == '\0');
     +	/*
    -+	 * Freshly-initialized strbufs may not have a dynamically allocated
    -+	 * buffer
    -+	 */
    -+	if (buf->len == 0 && buf->alloc == 0)
    -+		return;
    -+	/* alloc must be at least one byte larger than len */
    -+	cl_assert(buf->len < buf->alloc);
    ++         * In case the buffer contains anything, `alloc` must alloc must
    ++         * be at least one byte larger than `len`.
    ++         */
    ++	if (buf->len)
    ++            cl_assert(buf->len < buf->alloc);
     +}
     +
     +void test_strbuf__static_init(void)
    @@ t/unit-tests/u-strbuf.c (new)
     +	setup(t_addch, "");
     +}
     +
    -+void test_strbuf__add_multi_char(void)
    ++void test_strbuf__add_append_char(void)
     +{
     +	setup_populated(t_addch, "initial value", "a");
     +}
    @@ t/unit-tests/u-strbuf.c (new)
     +	setup(t_addstr, "hello there");
     +}
     +
    -+void test_strbuf__add_multi_str(void)
    ++void test_strbuf__add_append_str(void)
     +{
     +	setup_populated(t_addstr, "initial value", "hello there");
     +}
4:  f648cf4a4d ! 4:  2dde9110c2 t/unit-tests: convert strcmp-offset test to use clar test framework
    @@ Commit message
         t/unit-tests: convert strcmp-offset test to use clar test framework
     
         Adapt strcmp-offset test script to clar framework by using clar
    -    assertions where necessary. Test functions are created as standalone to
    -    test different test cases.
    +    assertions where necessary. Introduce `test_strcmp_offset__empty()` to
    +    verify `check_strcmp_offset()` behavior when both input strings are
    +    empty. This ensures the function correctly handles edge cases and
    +    returns expected values.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
-- 
2.47.0.86.g15030f9556

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

[Seyi Kuforiji]

Adapts hashmap test script to clar framework by using clar assertions
where necessary.

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-hashmap.c => u-hashmap.c} | 226 +++++++++++-----------
 3 files changed, 114 insertions(+), 116 deletions(-)
 rename t/unit-tests/{t-hashmap.c => u-hashmap.c} (60%)

diff --git a/Makefile b/Makefile
index b0cdc1fd4a..2d9dad119a 100644
--- a/Makefile
+++ b/Makefile
@@ -1339,6 +1339,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
 CLAR_TEST_SUITES += u-ctype
 CLAR_TEST_SUITES += u-hash
+CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
@@ -1349,7 +1350,6 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
 UNIT_TEST_PROGRAMS += t-example-decorate
-UNIT_TEST_PROGRAMS += t-hashmap
 UNIT_TEST_PROGRAMS += t-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
diff --git a/t/meson.build b/t/meson.build
index 14fea8dddf..af597f9804 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,6 +1,7 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
   'unit-tests/u-hash.c',
+  'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
@@ -45,7 +46,6 @@ test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
   'unit-tests/t-example-decorate.c',
-  'unit-tests/t-hashmap.c',
   'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
diff --git a/t/unit-tests/t-hashmap.c b/t/unit-tests/u-hashmap.c
similarity index 60%
rename from t/unit-tests/t-hashmap.c
rename to t/unit-tests/u-hashmap.c
index 83b79dff39..eb80aa1348 100644
--- a/t/unit-tests/t-hashmap.c
+++ b/t/unit-tests/u-hashmap.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "hashmap.h"
 #include "strbuf.h"
 
@@ -83,23 +83,23 @@ static void t_replace(struct hashmap *map, unsigned int ignore_case)
 	struct test_entry *entry;
 
 	entry = alloc_test_entry("key1", "value1", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 
 	entry = alloc_test_entry(ignore_case ? "Key1" : "key1", "value2",
 				 ignore_case);
 	entry = hashmap_put_entry(map, entry, ent);
-	if (check(entry != NULL))
-		check_str(get_value(entry), "value1");
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(get_value(entry), "value1");
 	free(entry);
 
 	entry = alloc_test_entry("fooBarFrotz", "value3", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 
 	entry = alloc_test_entry(ignore_case ? "FOObarFrotz" : "fooBarFrotz",
 				 "value4", ignore_case);
 	entry = hashmap_put_entry(map, entry, ent);
-	if (check(entry != NULL))
-		check_str(get_value(entry), "value3");
+	cl_assert(entry != NULL);
+	cl_assert_equal_s(get_value(entry), "value3");
 	free(entry);
 }
 
@@ -122,20 +122,18 @@ static void t_get(struct hashmap *map, unsigned int ignore_case)
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1],
 					 ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
 		entry = get_test_entry(map, query[i][0], ignore_case);
-		if (check(entry != NULL))
-			check_str(get_value(entry), query[i][1]);
-		else
-			test_msg("query key: %s", query[i][0]);
+		cl_assert(entry != NULL);
+		cl_assert_equal_s(get_value(entry), query[i][1]);
 	}
 
-	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
+	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
 }
 
 static void t_add(struct hashmap *map, unsigned int ignore_case)
@@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
 
 		hashmap_for_each_entry_from(map, entry, ent)
 		{
-			int ret;
-			if (!check_int((ret = key_val_contains(
-						key_val, seen,
-						ARRAY_SIZE(key_val), entry)),
-				       ==, 0)) {
-				switch (ret) {
-				case 1:
-					test_msg("found entry was not given in the input\n"
-						 "    key: %s\n  value: %s",
-						 entry->key, get_value(entry));
-					break;
-				case 2:
-					test_msg("duplicate entry detected\n"
-						 "    key: %s\n  value: %s",
-						 entry->key, get_value(entry));
-					break;
-				}
-			} else {
-				count++;
-			}
+			int ret = key_val_contains(key_val, seen,
+						   ARRAY_SIZE(key_val), entry);
+			cl_assert_equal_i(ret, 0);
+			count++;
 		}
-		check_int(count, ==, 2);
+		cl_assert_equal_i(count, 2);
 	}
 
-	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
-		if (!check_int(seen[i], ==, 1))
-			test_msg("following key-val pair was not iterated over:\n"
-				 "    key: %s\n  value: %s",
-				 key_val[i][0], key_val[i][1]);
-	}
+	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
+		cl_assert_equal_i(seen[i], 1);
 
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
-	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
+	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
 }
 
 static void t_remove(struct hashmap *map, unsigned int ignore_case)
@@ -211,24 +189,25 @@ static void t_remove(struct hashmap *map, unsigned int ignore_case)
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	for (size_t i = 0; i < ARRAY_SIZE(remove); i++) {
 		entry = alloc_test_entry(remove[i][0], "", ignore_case);
 		removed = hashmap_remove_entry(map, entry, ent, remove[i][0]);
-		if (check(removed != NULL))
-			check_str(get_value(removed), remove[i][1]);
+		cl_assert(removed != NULL);
+		cl_assert_equal_s(get_value(removed), remove[i][1]);
 		free(entry);
 		free(removed);
 	}
 
 	entry = alloc_test_entry("notInMap", "", ignore_case);
-	check_pointer_eq(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
+	cl_assert_equal_p(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
 	free(entry);
 
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map),
+			  ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
 }
 
 static void t_iterate(struct hashmap *map, unsigned int ignore_case)
@@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
 
 	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
 		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 	}
 
 	hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
 	{
-		int ret;
-		if (!check_int((ret = key_val_contains(key_val, seen,
-						       ARRAY_SIZE(key_val),
-						       entry)), ==, 0)) {
-			switch (ret) {
-			case 1:
-				test_msg("found entry was not given in the input\n"
-					 "    key: %s\n  value: %s",
-					 entry->key, get_value(entry));
-				break;
-			case 2:
-				test_msg("duplicate entry detected\n"
-					 "    key: %s\n  value: %s",
-					 entry->key, get_value(entry));
-				break;
-			}
-		}
+		int ret = key_val_contains(key_val, seen,
+					   ARRAY_SIZE(key_val),
+					   entry);
+		cl_assert(ret == 0);
 	}
 
-	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
-		if (!check_int(seen[i], ==, 1))
-			test_msg("following key-val pair was not iterated over:\n"
-				 "    key: %s\n  value: %s",
-				 key_val[i][0], key_val[i][1]);
-	}
+	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
+		cl_assert_equal_i(seen[i], 1);
 
-	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
+	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
 }
 
 static void t_alloc(struct hashmap *map, unsigned int ignore_case)
@@ -284,17 +246,17 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
 		char *key = xstrfmt("key%d", i);
 		char *value = xstrfmt("value%d", i);
 		entry = alloc_test_entry(key, value, ignore_case);
-		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
+		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
 		free(key);
 		free(value);
 	}
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, 51);
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), 51);
 
 	entry = alloc_test_entry("key52", "value52", ignore_case);
-	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
-	check_int(map->tablesize, ==, 256);
-	check_int(hashmap_get_size(map), ==, 52);
+	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
+	cl_assert_equal_i(map->tablesize, 256);
+	cl_assert_equal_i(hashmap_get_size(map), 52);
 
 	for (int i = 1; i <= 12; i++) {
 		char *key = xstrfmt("key%d", i);
@@ -302,27 +264,27 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
 
 		entry = alloc_test_entry(key, "", ignore_case);
 		removed = hashmap_remove_entry(map, entry, ent, key);
-		if (check(removed != NULL))
-			check_str(value, get_value(removed));
+		cl_assert(removed != NULL);
+		cl_assert_equal_s(value, get_value(removed));
 		free(key);
 		free(value);
 		free(entry);
 		free(removed);
 	}
-	check_int(map->tablesize, ==, 256);
-	check_int(hashmap_get_size(map), ==, 40);
+	cl_assert_equal_i(map->tablesize, 256);
+	cl_assert_equal_i(hashmap_get_size(map), 40);
 
 	entry = alloc_test_entry("key40", "", ignore_case);
 	removed = hashmap_remove_entry(map, entry, ent, "key40");
-	if (check(removed != NULL))
-		check_str("value40", get_value(removed));
-	check_int(map->tablesize, ==, 64);
-	check_int(hashmap_get_size(map), ==, 39);
+	cl_assert(removed != NULL);
+	cl_assert_equal_s("value40", get_value(removed));
+	cl_assert_equal_i(map->tablesize, 64);
+	cl_assert_equal_i(hashmap_get_size(map), 39);
 	free(entry);
 	free(removed);
 }
 
-static void t_intern(void)
+void test_hashmap__intern(void)
 {
 	const char *values[] = { "value1", "Value1", "value2", "value2" };
 
@@ -330,32 +292,68 @@ static void t_intern(void)
 		const char *i1 = strintern(values[i]);
 		const char *i2 = strintern(values[i]);
 
-		if (!check(!strcmp(i1, values[i])))
-			test_msg("strintern(%s) returns %s\n", values[i], i1);
-		else if (!check(i1 != values[i]))
-			test_msg("strintern(%s) returns input pointer\n",
-				 values[i]);
-		else if (!check_pointer_eq(i1, i2))
-			test_msg("address('%s') != address('%s'), so strintern('%s') != strintern('%s')",
-				 i1, i2, values[i], values[i]);
-		else
-			check_str(i1, values[i]);
+		cl_assert_equal_s(i1, values[i]);
+		cl_assert(i1 != values[i]);
+		cl_assert_equal_p(i1, i2);
 	}
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_hashmap__replace_case_sensitive(void)
+{
+	setup(t_replace, 0);
+}
+
+void test_hashmap__replace_case_insensitive(void)
+{
+	setup(t_replace, 1);
+}
+
+void test_hashmap__get_case_sensitive(void)
+{
+	setup(t_get, 0);
+}
+
+void test_hashmap__get_case_insensitive(void)
+{
+	setup(t_get, 1);
+}
+
+void test_hashmap__add_case_sensitive(void)
+{
+	setup(t_add, 0);
+}
+
+void test_hashmap__add_case_insensitive(void)
+{
+	setup(t_add, 1);
+}
+
+void test_hashmap__remove_case_sensitive(void)
+{
+	setup(t_remove, 0);
+}
+
+void test_hashmap__remove_case_insensitive(void)
+{
+	setup(t_remove, 1);
+}
+
+void test_hashmap__iterate_case_sensitive(void)
+{
+	setup(t_iterate, 0);
+}
+
+void test_hashmap__iterate_case_insensitive(void)
+{
+	setup(t_iterate, 1);
+}
+
+void test_hashmap__alloc_case_sensitive(void)
+{
+	setup(t_alloc, 0);
+}
+
+void test_hashmap__alloc_case_insensitive(void)
 {
-	TEST(setup(t_replace, 0), "replace works");
-	TEST(setup(t_replace, 1), "replace (case insensitive) works");
-	TEST(setup(t_get, 0), "get works");
-	TEST(setup(t_get, 1), "get (case insensitive) works");
-	TEST(setup(t_add, 0), "add works");
-	TEST(setup(t_add, 1), "add (case insensitive) works");
-	TEST(setup(t_remove, 0), "remove works");
-	TEST(setup(t_remove, 1), "remove (case insensitive) works");
-	TEST(setup(t_iterate, 0), "iterate works");
-	TEST(setup(t_iterate, 1), "iterate (case insensitive) works");
-	TEST(setup(t_alloc, 0), "grow / shrink works");
-	TEST(t_intern(), "string interning works");
-	return test_done();
+	setup(t_alloc, 1);
 }
-- 
2.47.0.86.g15030f9556

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

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> Adapts hashmap test script to clar framework by using clar assertions
> where necessary.
>
> 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-hashmap.c => u-hashmap.c} | 226 +++++++++++-----------
>  3 files changed, 114 insertions(+), 116 deletions(-)
>  rename t/unit-tests/{t-hashmap.c => u-hashmap.c} (60%)

The conversion looks fairly straight-forward.
Nice work.

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

[phillip.wood123]

Hi Seyi

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

As Patrick mentioned we use the imperative mood for our commit messages
so this should start "Adapt". I'm a bit confused by "where necessary" -
isn't that everywhere because we're using a different framework. Maybe
it would be clearer to say that we're replacing the existing assertions
with the equivalent clar assertions.

The conversion here looks correct but I'm concerned about the loss of
diagnostic messages.

> -	if (check(entry != NULL))
> -		check_str(get_value(entry), "value3");
> +	cl_assert(entry != NULL);
> +	cl_assert_equal_s(get_value(entry), "value3");

Unfortunately cl_assert_equal_s() is not equivalent to check_str()
because it does not handle NULL correctly. I think it would be very
helpful to fix that upstream. The diff below shows a possible solution
which avoids any ambiguity as to which pointer is NULL by only quoting
non-NULL values. In the long run it would be good to fix
cl_assert_equal_s() to properly quote control characters as check_str()
does.

diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
index d54e4553674..16f86c952f7 100644
--- a/t/unit-tests/clar/clar.c
+++ b/t/unit-tests/clar/clar.c
@@ -754,7 +754,12 @@ void clar__assert_equal(
                                  p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
                                          s1, s2, pos);
                          } else {
-                                p_snprintf(buf, sizeof(buf), "'%s' != '%s'", s1, s2);
+                                const char *q1 = s1 ? "'" : "";
+                                const char *q2 = s2 ? "'" : "";
+                                s1 = s1 ? s1 : "NULL";
+                                s2 = s2 ? s2 : "NULL";
+                                p_snprintf(buf, sizeof(buf), "%s%s%s != %s%s%s",
+                                           q1, s1, q1, q2, s2, q2);
                          }
                  }
          }
  
>   	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
>   		entry = get_test_entry(map, query[i][0], ignore_case);
> -		if (check(entry != NULL))
> -			check_str(get_value(entry), query[i][1]);
> -		else
> -			test_msg("query key: %s", query[i][0]);

It is a shame that we're removing all of the helpful debugging messages
from this test. It would be much nicer if we could keep them by using an
if statement and cl_failf() as we do in u-ctype.c

Best Wishes

Phillip

> +		cl_assert(entry != NULL);
> +		cl_assert_equal_s(get_value(entry), query[i][1]);
>   	}
>   
> -	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> +	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
>   }
>   
>   static void t_add(struct hashmap *map, unsigned int ignore_case)
> @@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
>   
>   		hashmap_for_each_entry_from(map, entry, ent)
>   		{
> -			int ret;
> -			if (!check_int((ret = key_val_contains(
> -						key_val, seen,
> -						ARRAY_SIZE(key_val), entry)),
> -				       ==, 0)) {
> -				switch (ret) {
> -				case 1:
> -					test_msg("found entry was not given in the input\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				case 2:
> -					test_msg("duplicate entry detected\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				}
> -			} else {
> -				count++;
> -			}
> +			int ret = key_val_contains(key_val, seen,
> +						   ARRAY_SIZE(key_val), entry);
> +			cl_assert_equal_i(ret, 0);
> +			count++;
>   		}
> -		check_int(count, ==, 2);
> +		cl_assert_equal_i(count, 2);
>   	}
>   
> -	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
> -		if (!check_int(seen[i], ==, 1))
> -			test_msg("following key-val pair was not iterated over:\n"
> -				 "    key: %s\n  value: %s",
> -				 key_val[i][0], key_val[i][1]);
> -	}
> +	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
> +		cl_assert_equal_i(seen[i], 1);
>   
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> -	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
> +	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
>   }
>   
>   static void t_remove(struct hashmap *map, unsigned int ignore_case)
> @@ -211,24 +189,25 @@ static void t_remove(struct hashmap *map, unsigned int ignore_case)
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
>   		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   	}
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(remove); i++) {
>   		entry = alloc_test_entry(remove[i][0], "", ignore_case);
>   		removed = hashmap_remove_entry(map, entry, ent, remove[i][0]);
> -		if (check(removed != NULL))
> -			check_str(get_value(removed), remove[i][1]);
> +		cl_assert(removed != NULL);
> +		cl_assert_equal_s(get_value(removed), remove[i][1]);
>   		free(entry);
>   		free(removed);
>   	}
>   
>   	entry = alloc_test_entry("notInMap", "", ignore_case);
> -	check_pointer_eq(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
> +	cl_assert_equal_p(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
>   	free(entry);
>   
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map),
> +			  ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
>   }
>   
>   static void t_iterate(struct hashmap *map, unsigned int ignore_case)
> @@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
>   		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   	}
>   
>   	hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
>   	{
> -		int ret;
> -		if (!check_int((ret = key_val_contains(key_val, seen,
> -						       ARRAY_SIZE(key_val),
> -						       entry)), ==, 0)) {
> -			switch (ret) {
> -			case 1:
> -				test_msg("found entry was not given in the input\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			case 2:
> -				test_msg("duplicate entry detected\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			}
> -		}
> +		int ret = key_val_contains(key_val, seen,
> +					   ARRAY_SIZE(key_val),
> +					   entry);
> +		cl_assert(ret == 0);
>   	}
>   
> -	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
> -		if (!check_int(seen[i], ==, 1))
> -			test_msg("following key-val pair was not iterated over:\n"
> -				 "    key: %s\n  value: %s",
> -				 key_val[i][0], key_val[i][1]);
> -	}
> +	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
> +		cl_assert_equal_i(seen[i], 1);
>   
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
>   }
>   
>   static void t_alloc(struct hashmap *map, unsigned int ignore_case)
> @@ -284,17 +246,17 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
>   		char *key = xstrfmt("key%d", i);
>   		char *value = xstrfmt("value%d", i);
>   		entry = alloc_test_entry(key, value, ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   		free(key);
>   		free(value);
>   	}
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, 51);
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), 51);
>   
>   	entry = alloc_test_entry("key52", "value52", ignore_case);
> -	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> -	check_int(map->tablesize, ==, 256);
> -	check_int(hashmap_get_size(map), ==, 52);
> +	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
> +	cl_assert_equal_i(map->tablesize, 256);
> +	cl_assert_equal_i(hashmap_get_size(map), 52);
>   
>   	for (int i = 1; i <= 12; i++) {
>   		char *key = xstrfmt("key%d", i);
> @@ -302,27 +264,27 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
>   
>   		entry = alloc_test_entry(key, "", ignore_case);
>   		removed = hashmap_remove_entry(map, entry, ent, key);
> -		if (check(removed != NULL))
> -			check_str(value, get_value(removed));
> +		cl_assert(removed != NULL);
> +		cl_assert_equal_s(value, get_value(removed));
>   		free(key);
>   		free(value);
>   		free(entry);
>   		free(removed);
>   	}
> -	check_int(map->tablesize, ==, 256);
> -	check_int(hashmap_get_size(map), ==, 40);
> +	cl_assert_equal_i(map->tablesize, 256);
> +	cl_assert_equal_i(hashmap_get_size(map), 40);
>   
>   	entry = alloc_test_entry("key40", "", ignore_case);
>   	removed = hashmap_remove_entry(map, entry, ent, "key40");
> -	if (check(removed != NULL))
> -		check_str("value40", get_value(removed));
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, 39);
> +	cl_assert(removed != NULL);
> +	cl_assert_equal_s("value40", get_value(removed));
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), 39);
>   	free(entry);
>   	free(removed);
>   }
>   
> -static void t_intern(void)
> +void test_hashmap__intern(void)
>   {
>   	const char *values[] = { "value1", "Value1", "value2", "value2" };
>   
> @@ -330,32 +292,68 @@ static void t_intern(void)
>   		const char *i1 = strintern(values[i]);
>   		const char *i2 = strintern(values[i]);
>   
> -		if (!check(!strcmp(i1, values[i])))
> -			test_msg("strintern(%s) returns %s\n", values[i], i1);
> -		else if (!check(i1 != values[i]))
> -			test_msg("strintern(%s) returns input pointer\n",
> -				 values[i]);
> -		else if (!check_pointer_eq(i1, i2))
> -			test_msg("address('%s') != address('%s'), so strintern('%s') != strintern('%s')",
> -				 i1, i2, values[i], values[i]);
> -		else
> -			check_str(i1, values[i]);
> +		cl_assert_equal_s(i1, values[i]);
> +		cl_assert(i1 != values[i]);
> +		cl_assert_equal_p(i1, i2);
>   	}
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_hashmap__replace_case_sensitive(void)
> +{
> +	setup(t_replace, 0);
> +}
> +
> +void test_hashmap__replace_case_insensitive(void)
> +{
> +	setup(t_replace, 1);
> +}
> +
> +void test_hashmap__get_case_sensitive(void)
> +{
> +	setup(t_get, 0);
> +}
> +
> +void test_hashmap__get_case_insensitive(void)
> +{
> +	setup(t_get, 1);
> +}
> +
> +void test_hashmap__add_case_sensitive(void)
> +{
> +	setup(t_add, 0);
> +}
> +
> +void test_hashmap__add_case_insensitive(void)
> +{
> +	setup(t_add, 1);
> +}
> +
> +void test_hashmap__remove_case_sensitive(void)
> +{
> +	setup(t_remove, 0);
> +}
> +
> +void test_hashmap__remove_case_insensitive(void)
> +{
> +	setup(t_remove, 1);
> +}
> +
> +void test_hashmap__iterate_case_sensitive(void)
> +{
> +	setup(t_iterate, 0);
> +}
> +
> +void test_hashmap__iterate_case_insensitive(void)
> +{
> +	setup(t_iterate, 1);
> +}
> +
> +void test_hashmap__alloc_case_sensitive(void)
> +{
> +	setup(t_alloc, 0);
> +}
> +
> +void test_hashmap__alloc_case_insensitive(void)
>   {
> -	TEST(setup(t_replace, 0), "replace works");
> -	TEST(setup(t_replace, 1), "replace (case insensitive) works");
> -	TEST(setup(t_get, 0), "get works");
> -	TEST(setup(t_get, 1), "get (case insensitive) works");
> -	TEST(setup(t_add, 0), "add works");
> -	TEST(setup(t_add, 1), "add (case insensitive) works");
> -	TEST(setup(t_remove, 0), "remove works");
> -	TEST(setup(t_remove, 1), "remove (case insensitive) works");
> -	TEST(setup(t_iterate, 0), "iterate works");
> -	TEST(setup(t_iterate, 1), "iterate (case insensitive) works");
> -	TEST(setup(t_alloc, 0), "grow / shrink works");
> -	TEST(t_intern(), "string interning works");
> -	return test_done();
> +	setup(t_alloc, 1);
>   }

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

[Patrick Steinhardt]

On Sun, Feb 02, 2025 at 11:09:25AM +0000, phillip.wood123@gmail.com wrote:
> On 31/01/2025 22:14, Seyi Kuforiji wrote:
> > -	if (check(entry != NULL))
> > -		check_str(get_value(entry), "value3");
> > +	cl_assert(entry != NULL);
> > +	cl_assert_equal_s(get_value(entry), "value3");
> 
> Unfortunately cl_assert_equal_s() is not equivalent to check_str()
> because it does not handle NULL correctly. I think it would be very
> helpful to fix that upstream. The diff below shows a possible solution
> which avoids any ambiguity as to which pointer is NULL by only quoting
> non-NULL values. In the long run it would be good to fix
> cl_assert_equal_s() to properly quote control characters as check_str()
> does.

That's indeed something we should fix in clar.

> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
> index d54e4553674..16f86c952f7 100644
> --- a/t/unit-tests/clar/clar.c
> +++ b/t/unit-tests/clar/clar.c
> @@ -754,7 +754,12 @@ void clar__assert_equal(
>                                  p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
>                                          s1, s2, pos);
>                          } else {
> -                                p_snprintf(buf, sizeof(buf), "'%s' != '%s'", s1, s2);
> +                                const char *q1 = s1 ? "'" : "";
> +                                const char *q2 = s2 ? "'" : "";
> +                                s1 = s1 ? s1 : "NULL";
> +                                s2 = s2 ? s2 : "NULL";
> +                                p_snprintf(buf, sizeof(buf), "%s%s%s != %s%s%s",
> +                                           q1, s1, q1, q2, s2, q2);
>                          }
>                  }
>          }

Would you mind creating an upstream pull request with these changes? I'm
happy to review, and then we can update our embedded version of clar.

> >   	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
> >   		entry = get_test_entry(map, query[i][0], ignore_case);
> > -		if (check(entry != NULL))
> > -			check_str(get_value(entry), query[i][1]);
> > -		else
> > -			test_msg("query key: %s", query[i][0]);
> 
> It is a shame that we're removing all of the helpful debugging messages
> from this test. It would be much nicer if we could keep them by using an
> if statement and cl_failf() as we do in u-ctype.c

I honestly think that the debug messages don't add much and only add to
the noise. You shouldn't ever see them, and if you do something is
broken and you'll likely end up pulling out the debugger anyway. So I'm
more in the camp of writing unit tests in a concise way rather than the
needlessly-verbose style we previously had.

Patrick

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

[phillip.wood123]

Hi Patrick

On 03/02/2025 07:30, Patrick Steinhardt wrote:
> On Sun, Feb 02, 2025 at 11:09:25AM +0000, phillip.wood123@gmail.com wrote:
>> On 31/01/2025 22:14, Seyi Kuforiji wrote:
> 
>> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
>> index d54e4553674..16f86c952f7 100644
>> --- a/t/unit-tests/clar/clar.c
>> +++ b/t/unit-tests/clar/clar.c
>> @@ -754,7 +754,12 @@ void clar__assert_equal(
>>                                   p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
>>                                           s1, s2, pos);
>>                           } else {
>> -                                p_snprintf(buf, sizeof(buf), "'%s' != '%s'", s1, s2);
>> +                                const char *q1 = s1 ? "'" : "";
>> +                                const char *q2 = s2 ? "'" : "";
>> +                                s1 = s1 ? s1 : "NULL";
>> +                                s2 = s2 ? s2 : "NULL";
>> +                                p_snprintf(buf, sizeof(buf), "%s%s%s != %s%s%s",
>> +                                           q1, s1, q1, q2, s2, q2);
>>                           }
>>                   }
>>           }
> 
> Would you mind creating an upstream pull request with these changes? I'm
> happy to review, and then we can update our embedded version of clar.

I've opened a PR at https://github.com/clar-test/clar/pull/114

>>>    	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
>>>    		entry = get_test_entry(map, query[i][0], ignore_case);
>>> -		if (check(entry != NULL))
>>> -			check_str(get_value(entry), query[i][1]);
>>> -		else
>>> -			test_msg("query key: %s", query[i][0]);
>>
>> It is a shame that we're removing all of the helpful debugging messages
>> from this test. It would be much nicer if we could keep them by using an
>> if statement and cl_failf() as we do in u-ctype.c
> 
> I honestly think that the debug messages don't add much and only add to
> the noise. You shouldn't ever see them, and if you do something is
> broken and you'll likely end up pulling out the debugger anyway. So I'm
> more in the camp of writing unit tests in a concise way rather than the
> needlessly-verbose style we previously had.

If I'm firing up the debugger I'd rather as much detail as I can about 
what went wrong so I can see where to set my breakpoints. Otherwise I 
need to waste time repeating the test to find out exactly what went 
wrong before I can make any progress. My experience with debugging our 
integration tests is that those tests that take care to print helpful 
diagnostic messages when they fail are a lot easier to debug than those 
that don't.

Best Wishes

Phillip

[PATCH v2 2/4] t/unit-tests: adapt example decorate test to use clar test framework

[Seyi Kuforiji]

Introduce `test_example_decorate__initialize()` to explicitly set up
object IDs and retrieve corresponding objects before tests run. This
ensures a consistent and predictable test state without relying on data
from previous tests.

Add `test_example_decorate__cleanup()` to clear decorations after each
test, preventing interference between tests and ensuring each runs in
isolation.

Adapt example decorate test script to clar framework by using clar
assertions where necessary. Previously, tests relied on data written by
earlier tests, leading to unintended dependencies between them. This
explicitly initializes the necessary state within
`test_example_decorate__readd`, ensuring it does not depend on prior
test executions.

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

diff --git a/Makefile b/Makefile
index 2d9dad119a..732d765f1c 100644
--- a/Makefile
+++ b/Makefile
@@ -1338,6 +1338,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
 THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
 CLAR_TEST_SUITES += u-ctype
+CLAR_TEST_SUITES += u-example-decorate
 CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
@@ -1349,7 +1350,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-example-decorate
 UNIT_TEST_PROGRAMS += t-oid-array
 UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
diff --git a/t/meson.build b/t/meson.build
index af597f9804..c7e08eca6f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,5 +1,6 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
+  'unit-tests/u-example-decorate.c',
   'unit-tests/u-hash.c',
   'unit-tests/u-hashmap.c',
   'unit-tests/u-mem-pool.c',
@@ -45,7 +46,6 @@ clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-example-decorate.c',
   'unit-tests/t-oid-array.c',
   'unit-tests/t-oidmap.c',
   'unit-tests/t-oidtree.c',
diff --git a/t/unit-tests/t-example-decorate.c b/t/unit-tests/u-example-decorate.c
similarity index 30%
rename from t/unit-tests/t-example-decorate.c
rename to t/unit-tests/u-example-decorate.c
index bfc776e223..9b1d1ce753 100644
--- a/t/unit-tests/t-example-decorate.c
+++ b/t/unit-tests/u-example-decorate.c
@@ -1,6 +1,6 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
-#include "test-lib.h"
+#include "unit-test.h"
 #include "object.h"
 #include "decorate.h"
 #include "repository.h"
@@ -11,64 +11,54 @@ struct test_vars {
 	int decoration_a, decoration_b;
 };
 
-static void t_add(struct test_vars *vars)
+static struct test_vars vars;
+
+void test_example_decorate__initialize(void)
 {
-	void *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);
+	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
 
-	check(ret == NULL);
-	ret = add_decoration(&vars->n, vars->two, NULL);
-	check(ret == NULL);
+	vars.one = lookup_unknown_object(the_repository, &one_oid);
+	vars.two = lookup_unknown_object(the_repository, &two_oid);
+	vars.three = lookup_unknown_object(the_repository, &three_oid);
 }
 
-static void t_readd(struct test_vars *vars)
+void test_example_decorate__cleanup(void)
 {
-	void *ret = add_decoration(&vars->n, vars->one, NULL);
-
-	check(ret == &vars->decoration_a);
-	ret = add_decoration(&vars->n, vars->two, &vars->decoration_b);
-	check(ret == NULL);
+	clear_decoration(&vars.n, NULL);
 }
 
-static void t_lookup(struct test_vars *vars)
+void test_example_decorate__add(void)
 {
-	void *ret = lookup_decoration(&vars->n, vars->one);
-
-	check(ret == NULL);
-	ret = lookup_decoration(&vars->n, vars->two);
-	check(ret == &vars->decoration_b);
-	ret = lookup_decoration(&vars->n, vars->three);
-	check(ret == NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
 }
 
-static void t_loop(struct test_vars *vars)
+void test_example_decorate__readd(void)
 {
-	int objects_noticed = 0;
-
-	for (size_t i = 0; i < vars->n.size; i++) {
-		if (vars->n.entries[i].base)
-			objects_noticed++;
-	}
-	check_int(objects_noticed, ==, 2);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.one, NULL), &vars.decoration_a);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_example_decorate__lookup(void)
 {
-	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
-	struct test_vars vars = { 0 };
+	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.one, NULL), NULL);
+	cl_assert_equal_p(lookup_decoration(&vars.n, vars.two), &vars.decoration_b);
+	cl_assert_equal_p(lookup_decoration(&vars.n, vars.one), NULL);
+}
 
-	vars.one = lookup_unknown_object(the_repository, &one_oid);
-	vars.two = lookup_unknown_object(the_repository, &two_oid);
-	vars.three = lookup_unknown_object(the_repository, &three_oid);
+void test_example_decorate__loop(void)
+{
+	int objects_noticed = 0;
 
-	TEST(t_add(&vars),
-	     "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.");
-	TEST(t_readd(&vars),
-	     "When re-adding an already existing object, the old decoration is returned.");
-	TEST(t_lookup(&vars),
-	     "Lookup returns the added declarations, or NULL if the object was never added.");
-	TEST(t_loop(&vars), "The user can also loop through all entries.");
+	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
+	cl_assert_equal_p(add_decoration(&vars.n, vars.two, &vars.decoration_b), NULL);
 
-	clear_decoration(&vars.n, NULL);
+	for (size_t i = 0; i < vars.n.size; i++)
+		if (vars.n.entries[i].base)
+			objects_noticed++;
 
-	return test_done();
+	cl_assert_equal_i(objects_noticed, 2);
 }
-- 
2.47.0.86.g15030f9556

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

[Seyi Kuforiji]

Adapt strbuf test script to clar framework by using clar assertions
where necessary.

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-strbuf.c => u-strbuf.c} | 115 ++++++++++++------------
 3 files changed, 58 insertions(+), 61 deletions(-)
 rename t/unit-tests/{t-strbuf.c => u-strbuf.c} (35%)

diff --git a/Makefile b/Makefile
index 732d765f1c..358193597f 100644
--- a/Makefile
+++ b/Makefile
@@ -1344,6 +1344,7 @@ CLAR_TEST_SUITES += u-hashmap
 CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
+CLAR_TEST_SUITES += u-strbuf
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
@@ -1361,7 +1362,6 @@ UNIT_TEST_PROGRAMS += t-reftable-reader
 UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-reftable-stack
-UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
diff --git a/t/meson.build b/t/meson.build
index c7e08eca6f..6cb72842b1 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-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
+  'unit-tests/u-strbuf.c',
   'unit-tests/u-strvec.c',
 ]
 
@@ -57,7 +58,6 @@ unit_test_programs = [
   'unit-tests/t-reftable-readwrite.c',
   'unit-tests/t-reftable-record.c',
   'unit-tests/t-reftable-stack.c',
-  'unit-tests/t-strbuf.c',
   'unit-tests/t-strcmp-offset.c',
   'unit-tests/t-trailer.c',
   'unit-tests/t-urlmatch-normalization.c',
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/u-strbuf.c
similarity index 35%
rename from t/unit-tests/t-strbuf.c
rename to t/unit-tests/u-strbuf.c
index 3f4044d697..caa5d78aa3 100644
--- a/t/unit-tests/t-strbuf.c
+++ b/t/unit-tests/u-strbuf.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "strbuf.h"
 
 /* wrapper that supplies tests with an empty, initialized strbuf */
@@ -9,8 +9,8 @@ static void setup(void (*f)(struct strbuf*, const void*),
 
 	f(&buf, data);
 	strbuf_release(&buf);
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
 }
 
 /* wrapper that supplies tests with a populated, initialized strbuf */
@@ -20,49 +20,45 @@ static void setup_populated(void (*f)(struct strbuf*, const void*),
 	struct strbuf buf = STRBUF_INIT;
 
 	strbuf_addstr(&buf, init_str);
-	check_uint(buf.len, ==, strlen(init_str));
+	cl_assert_equal_i(buf.len, strlen(init_str));
 	f(&buf, data);
 	strbuf_release(&buf);
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
 }
 
-static int assert_sane_strbuf(struct strbuf *buf)
+static void assert_sane_strbuf(struct strbuf *buf)
 {
 	/* Initialized strbufs should always have a non-NULL buffer */
-	if (!check(!!buf->buf))
-		return 0;
+	cl_assert(buf->buf != NULL);
 	/* Buffers should always be NUL-terminated */
-	if (!check_char(buf->buf[buf->len], ==, '\0'))
-		return 0;
+	cl_assert(buf->buf[buf->len] == '\0');
 	/*
-	 * Freshly-initialized strbufs may not have a dynamically allocated
-	 * buffer
-	 */
-	if (buf->len == 0 && buf->alloc == 0)
-		return 1;
-	/* alloc must be at least one byte larger than len */
-	return check_uint(buf->len, <, buf->alloc);
+         * In case the buffer contains anything, `alloc` must alloc must
+         * be at least one byte larger than `len`.
+         */
+	if (buf->len)
+            cl_assert(buf->len < buf->alloc);
 }
 
-static void t_static_init(void)
+void test_strbuf__static_init(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, ==, 0);
-	check_char(buf.buf[0], ==, '\0');
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert_equal_i(buf.alloc, 0);
+	cl_assert(buf.buf[0] == '\0');
 }
 
-static void t_dynamic_init(void)
+void test_strbuf__dynamic_init(void)
 {
 	struct strbuf buf;
 
 	strbuf_init(&buf, 1024);
-	check(assert_sane_strbuf(&buf));
-	check_uint(buf.len, ==, 0);
-	check_uint(buf.alloc, >=, 1024);
-	check_char(buf.buf[0], ==, '\0');
+	assert_sane_strbuf(&buf);
+	cl_assert_equal_i(buf.len, 0);
+	cl_assert(buf.alloc >= 1024);
+	cl_assert(buf.buf[0] == '\0');
 	strbuf_release(&buf);
 }
 
@@ -73,16 +69,12 @@ static void t_addch(struct strbuf *buf, const void *data)
 	size_t orig_alloc = buf->alloc;
 	size_t orig_len = buf->len;
 
-	if (!check(assert_sane_strbuf(buf)))
-		return;
+	assert_sane_strbuf(buf);
 	strbuf_addch(buf, ch);
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	if (!(check_uint(buf->len, ==, orig_len + 1) &&
-	      check_uint(buf->alloc, >=, orig_alloc)))
-		return; /* avoid de-referencing buf->buf */
-	check_char(buf->buf[buf->len - 1], ==, ch);
-	check_char(buf->buf[buf->len], ==, '\0');
+	assert_sane_strbuf(buf);
+	cl_assert_equal_i(buf->len, orig_len + 1);
+	cl_assert(buf->alloc >= orig_alloc);
+	cl_assert(buf->buf[buf->len] == '\0');
 }
 
 static void t_addstr(struct strbuf *buf, const void *data)
@@ -92,31 +84,36 @@ static void t_addstr(struct strbuf *buf, const void *data)
 	size_t orig_alloc = buf->alloc;
 	size_t orig_len = buf->len;
 
-	if (!check(assert_sane_strbuf(buf)))
-		return;
+	assert_sane_strbuf(buf);
 	strbuf_addstr(buf, text);
-	if (!check(assert_sane_strbuf(buf)))
-		return;
-	if (!(check_uint(buf->len, ==, orig_len + len) &&
-	      check_uint(buf->alloc, >=, orig_alloc) &&
-	      check_uint(buf->alloc, >, orig_len + len) &&
-	      check_char(buf->buf[orig_len + len], ==, '\0')))
-	    return;
-	check_str(buf->buf + orig_len, text);
+	assert_sane_strbuf(buf);
+	cl_assert_equal_i(buf->len, orig_len + len);
+	cl_assert(buf->alloc >= orig_alloc);
+	cl_assert(buf->buf[buf->len] == '\0');
+	cl_assert_equal_s(buf->buf + orig_len, text);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_strbuf__add_single_char(void)
 {
-	if (!TEST(t_static_init(), "static initialization works"))
-		test_skip_all("STRBUF_INIT is broken");
-	TEST(t_dynamic_init(), "dynamic initialization works");
-	TEST(setup(t_addch, "a"), "strbuf_addch adds char");
-	TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
-	TEST(setup_populated(t_addch, "initial value", "a"),
-	     "strbuf_addch appends to initial value");
-	TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
-	TEST(setup_populated(t_addstr, "initial value", "hello there"),
-	     "strbuf_addstr appends string to initial value");
-
-	return test_done();
+	setup(t_addch, "a");
+}
+
+void test_strbuf__add_empty_char(void)
+{
+	setup(t_addch, "");
+}
+
+void test_strbuf__add_append_char(void)
+{
+	setup_populated(t_addch, "initial value", "a");
+}
+
+void test_strbuf__add_single_str(void)
+{
+	setup(t_addstr, "hello there");
+}
+
+void test_strbuf__add_append_str(void)
+{
+	setup_populated(t_addstr, "initial value", "hello there");
 }
-- 
2.47.0.86.g15030f9556

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

[Phillip Wood]

Hi Seyi

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

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

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

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

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

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

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

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

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

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

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

	else
		cl_assert_equal_i(buf->alloc, 0);

To check that alloc is zero when len is zero.

Best Wishes

Phillip

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

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

[Seyi Kuforiji]

Adapt strcmp-offset test script to clar framework by using clar
assertions where necessary. Introduce `test_strcmp_offset__empty()` to
verify `check_strcmp_offset()` behavior when both input strings are
empty. This ensures the function correctly handles edge cases and
returns expected values.

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

diff --git a/Makefile b/Makefile
index 358193597f..76b5de4fdd 100644
--- a/Makefile
+++ b/Makefile
@@ -1345,6 +1345,7 @@ CLAR_TEST_SUITES += u-mem-pool
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
+CLAR_TEST_SUITES += u-strcmp-offset
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
@@ -1362,7 +1363,6 @@ UNIT_TEST_PROGRAMS += t-reftable-reader
 UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-reftable-stack
-UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGRAMS += t-urlmatch-normalization
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
diff --git a/t/meson.build b/t/meson.build
index 6cb72842b1..3935782bbb 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -7,6 +7,7 @@ clar_test_suites = [
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
+  'unit-tests/u-strcmp-offset.c',
   'unit-tests/u-strvec.c',
 ]
 
@@ -58,7 +59,6 @@ unit_test_programs = [
   'unit-tests/t-reftable-readwrite.c',
   'unit-tests/t-reftable-record.c',
   'unit-tests/t-reftable-stack.c',
-  'unit-tests/t-strcmp-offset.c',
   'unit-tests/t-trailer.c',
   'unit-tests/t-urlmatch-normalization.c',
 ]
diff --git a/t/unit-tests/t-strcmp-offset.c b/t/unit-tests/u-strcmp-offset.c
similarity index 39%
rename from t/unit-tests/t-strcmp-offset.c
rename to t/unit-tests/u-strcmp-offset.c
index 6880f21161..7e8e9acf3c 100644
--- a/t/unit-tests/t-strcmp-offset.c
+++ b/t/unit-tests/u-strcmp-offset.c
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "unit-test.h"
 #include "read-cache-ll.h"
 
 static void check_strcmp_offset(const char *string1, const char *string2,
@@ -15,21 +15,31 @@ static void check_strcmp_offset(const char *string1, const char *string2,
 			result > 0 ? 1 :
 			0);
 
-	check_int(result, ==, expect_result);
-	check_uint((uintmax_t)offset, ==, expect_offset);
+	cl_assert_equal_i(result, expect_result);
+	cl_assert_equal_i((uintmax_t)offset, expect_offset);
 }
 
-#define TEST_STRCMP_OFFSET(string1, string2, expect_result, expect_offset) \
-	TEST(check_strcmp_offset(string1, string2, expect_result,          \
-				 expect_offset),                           \
-	     "strcmp_offset(%s, %s) works", #string1, #string2)
+void test_strcmp_offset__empty(void)
+{
+	check_strcmp_offset("", "", 0, 0);
+}
+
+void test_strcmp_offset__equal(void)
+{
+	check_strcmp_offset("abc", "abc", 0, 3);
+}
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_strcmp_offset__different(void)
 {
-	TEST_STRCMP_OFFSET("abc", "abc", 0, 3);
-	TEST_STRCMP_OFFSET("abc", "def", -1, 0);
-	TEST_STRCMP_OFFSET("abc", "abz", -1, 2);
-	TEST_STRCMP_OFFSET("abc", "abcdef", -1, 3);
+	check_strcmp_offset("abc", "def", -1, 0);
+}
 
-	return test_done();
+void test_strcmp_offset__mismatch(void)
+{
+	check_strcmp_offset("abc", "abz", -1, 2);
+}
+
+void test_strcmp_offset__different_length(void)
+{
+	check_strcmp_offset("abc", "abcdef", -1, 3);
 }
-- 
2.47.0.86.g15030f9556

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

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> Hello,
>
> This small patch series transitions the existing unit test files to the
> Clar testing framework. This change is part of our ongoing effort to
> standardize our testing approach and enhance maintainability.
>
> Changes in v2:
> - small fixes to the commit messages and how they read
> - some small code fix up and refactoring
>
> Thanks
> Seyi
>
> Mentored-by: Patrick Steinhardt ps@pks.im
> Signed-off-by: Seyi Kuforiji kuforiji98@gmail.com
>
> Seyi Kuforiji (4):
>   t/unit-tests: convert hashmap test to use clar test framework
>   t/unit-tests: adapt example decorate test to use clar test framework
>   t/unit-tests: convert strbuf test to use clar test framework
>   t/unit-tests: convert strcmp-offset test to use clar test framework

Overall they looked quite straight-forward rewrite.  Nicely done.

Queued with automated fix-ups, so there is no need to resend only to
fix below.

Thanks.

Applying: t/unit-tests: convert hashmap test to use clar test framework
Applying: t/unit-tests: adapt example decorate test to use clar test framework
.git/rebase-apply/patch:105: indent with spaces.
         * In case the buffer contains anything, `alloc` must alloc must
.git/rebase-apply/patch:106: indent with spaces.
         * be at least one byte larger than `len`.
.git/rebase-apply/patch:107: indent with spaces.
         */
.git/rebase-apply/patch:109: indent with spaces.
            cl_assert(buf->len < buf->alloc);
warning: 4 lines add whitespace errors.
Applying: t/unit-tests: convert strbuf test to use clar test framework
Applying: t/unit-tests: convert strcmp-offset test to use clar test framework