threads / patch / 62873

patch, 4 partst/unit-tests: convert unit-tests to use clar

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

## tl;dr

21 messages between Jan 30, 2025 and Feb 3, 2025. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Seyi Kuforiji· Jan 30, 2025, 09:13 UTC · lore
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
Seyi Kuforiji· Jan 30, 2025, 09:13 UTC · re: Seyi Kuforiji · lore

[PATCH 1/4] t/unit-tests: convert hashmap test to clar 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.

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%)
Show changes to 4 files +114 −116

Makefile, t/meson.build, t/unit-tests/t-hashmap.c, t/unit-tests/u-hashmap.c

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
Patrick Steinhardt· Jan 31, 2025, 11:43 UTC · re: Seyi Kuforiji · lore

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

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 :)

Show 26 quoted lines
> 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.

Show 27 quoted lines
> @@ -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.

Show 30 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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
Seyi Kuforiji· Jan 30, 2025, 09:13 UTC · re: Seyi Kuforiji · lore

[PATCH 2/4] t/unit-tests: adapt example decorate test to clar 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.

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
Show changes to 4 files +78 −76

Makefile, t/meson.build, t/unit-tests/t-example-decorate.c, 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
Patrick Steinhardt· Jan 31, 2025, 11:43 UTC · re: Seyi Kuforiji · lore

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

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.

Show 158 quoted lines
> 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.

Show 6 quoted lines
> +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.

Show 30 quoted lines
> +	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.
Show 16 quoted lines
> +	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
Seyi Kuforiji· Jan 30, 2025, 09:13 UTC · re: Seyi Kuforiji · lore

[PATCH 3/4] t/unit-tests: convert strbuf test to clar 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.

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
Show changes to 4 files +123 −124

Makefile, t/meson.build, t/unit-tests/t-strbuf.c, 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
Patrick Steinhardt· Jan 31, 2025, 11:43 UTC · re: Seyi Kuforiji · lore

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

On Thu, Jan 30, 2025 at 10:13:33AM +0100, Seyi Kuforiji wrote:
> Adapt strbuf test script to clar framework by using clar assertions
> where necessary. Test functions are created as standalone to test
> different test cases.
Same comment here regarding the second sentence.
Show 8 quoted lines
> diff --git a/t/unit-tests/u-strbuf.c b/t/unit-tests/u-strbuf.c
> new file mode 100644
> index 0000000000..fec3c768d2
> --- /dev/null
> +++ b/t/unit-tests/u-strbuf.c
> @@ -0,0 +1,121 @@
> +#include "unit-test.h"
> +#include "strbuf.h"

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

[snip]
Show 14 quoted lines
> +static void assert_sane_strbuf(struct strbuf *buf)
> +{
> +	/* Initialized strbufs should always have a non-NULL buffer */
> +	cl_assert(buf->buf != NULL);
> +	/* Buffers should always be NUL-terminated */
> +	cl_assert(buf->buf[buf->len] == '\0');
> +	/*
> +	 * Freshly-initialized strbufs may not have a dynamically allocated
> +	 * buffer
> +	 */
> +	if (buf->len == 0 && buf->alloc == 0)
> +		return;
> +	/* alloc must be at least one byte larger than len */
> +	cl_assert(buf->len < buf->alloc);
I think this condition can be simplified a bit:
        /*
         * In case the buffer contains anything, `alloc` must alloc must
         * be at least one byte larger than `len`.
         */
        if (buf->len)
            cl_assert(buf->len < buf->alloc);
Show 6 quoted lines
> +void test_strbuf__add_empty_char(void)
> +{
> +	setup(t_addch, "");
> +}
> +
> +void test_strbuf__add_multi_char(void)
Nit: I think "add_multi_char" is slightly misleading. What this test
exercises is that we can append to a non-empty buffer. So maybe
"append_char" would be a better fit?
Show 10 quoted lines
> +{
> +	setup_populated(t_addch, "initial value", "a");
> +}
> +
> +void test_strbuf__add_single_str(void)
> +{
> +	setup(t_addstr, "hello there");
> +}
> +
> +void test_strbuf__add_multi_str(void)
Similarly, we could name this "append_str".
> +{
> +	setup_populated(t_addstr, "initial value", "hello there");
> +}
Patrick
Seyi Kuforiji· Jan 30, 2025, 09:13 UTC · re: Seyi Kuforiji · lore

[PATCH 4/4] t/unit-tests: convert strcmp-offset test to clar 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.

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
Show changes to 4 files +47 −37

Makefile, t/meson.build, t/unit-tests/t-strcmp-offset.c, 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
Patrick Steinhardt· Jan 31, 2025, 11:44 UTC · re: Seyi Kuforiji · lore

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

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.
Show 30 quoted lines
> 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
Patrick Steinhardt· Jan 31, 2025, 11:43 UTC · re: Seyi Kuforiji · lore

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

On Thu, Jan 30, 2025 at 10:13:30AM +0100, Seyi Kuforiji wrote:
Show 5 quoted lines
> 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
Seyi Kuforiji· Jan 31, 2025, 22:14 UTC · re: Seyi Kuforiji · lore

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

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
Seyi Kuforiji· Jan 31, 2025, 22:14 UTC · re: Seyi Kuforiji · lore

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

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%)
Show changes to 4 files +114 −116

Makefile, t/meson.build, t/unit-tests/t-hashmap.c, t/unit-tests/u-hashmap.c

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
Junio C Hamano· Jan 31, 2025, 23:03 UTC · re: Seyi Kuforiji · lore

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:
Show 11 quoted lines
> 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.

phillip.wood123@gmail.com· Feb 2, 2025, 11:09 UTC · re: Seyi Kuforiji · lore

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

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.

Show changes to t/unit-tests/clar/clar.c +6 −1
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);
>   }
Patrick Steinhardt· Feb 3, 2025, 07:30 UTC · re: phillip.wood123@gmail.com · lore

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

On Sun, Feb 02, 2025 at 11:09:25AM +0000, phillip.wood123@gmail.com wrote:
Show 13 quoted lines
> 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.
Show 18 quoted lines
> 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.

Show 10 quoted lines
> >   	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
phillip.wood123@gmail.com· Feb 3, 2025, 14:56 UTC · re: Patrick Steinhardt · lore

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

Hi Patrick
On 03/02/2025 07:30, Patrick Steinhardt wrote:
Show 24 quoted lines
> 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
Show 16 quoted lines
>>>    	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
Seyi Kuforiji· Jan 31, 2025, 22:14 UTC · re: Seyi Kuforiji · lore

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

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%)
Show changes to 4 files +35 −45

Makefile, t/meson.build, t/unit-tests/t-example-decorate.c, 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/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
Seyi Kuforiji· Jan 31, 2025, 22:14 UTC · re: Seyi Kuforiji · lore

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

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%)
Show changes to 4 files +58 −61

Makefile, t/meson.build, t/unit-tests/t-strbuf.c, 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/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
Phillip Wood· Feb 2, 2025, 14:38 UTC · re: Seyi Kuforiji · lore

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

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.

Show 9 quoted lines
>   /* 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.

Show 11 quoted lines
> -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.

Show 13 quoted lines
>   	/*
> -	 * 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
Show 114 quoted lines
>   }
>   
> -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");
>   }
Seyi Kuforiji· Jan 31, 2025, 22:14 UTC · re: Seyi Kuforiji · lore

[PATCH v2 4/4] 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. 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%)
Show changes to 4 files +25 −15

Makefile, t/meson.build, t/unit-tests/t-strcmp-offset.c, 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/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
Junio C Hamano· Jan 31, 2025, 23:06 UTC · re: Seyi Kuforiji · lore

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:
Show 21 quoted lines
> 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

← back to recent threads