git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Pphillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date
Feb 2, 2025, 11:09 UTC
Message-ID
<6be977a0-4bf9-4568-9b28-cdc988a49b89@gmail.com>
In-Reply-To
<20250131221420.38161-2-kuforiji98@gmail.com>
Hi Seyi
On 31/01/2025 22:14, Seyi Kuforiji wrote:
> Adapts hashmap test script to clar framework by using clar assertions
> where necessary.

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

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

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

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

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

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

Best Wishes

Phillip

> +		cl_assert(entry != NULL);
> +		cl_assert_equal_s(get_value(entry), query[i][1]);
>   	}
>   
> -	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> +	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
>   }
>   
>   static void t_add(struct hashmap *map, unsigned int ignore_case)
> @@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
>   
>   		hashmap_for_each_entry_from(map, entry, ent)
>   		{
> -			int ret;
> -			if (!check_int((ret = key_val_contains(
> -						key_val, seen,
> -						ARRAY_SIZE(key_val), entry)),
> -				       ==, 0)) {
> -				switch (ret) {
> -				case 1:
> -					test_msg("found entry was not given in the input\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				case 2:
> -					test_msg("duplicate entry detected\n"
> -						 "    key: %s\n  value: %s",
> -						 entry->key, get_value(entry));
> -					break;
> -				}
> -			} else {
> -				count++;
> -			}
> +			int ret = key_val_contains(key_val, seen,
> +						   ARRAY_SIZE(key_val), entry);
> +			cl_assert_equal_i(ret, 0);
> +			count++;
>   		}
> -		check_int(count, ==, 2);
> +		cl_assert_equal_i(count, 2);
>   	}
>   
> -	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
> -		if (!check_int(seen[i], ==, 1))
> -			test_msg("following key-val pair was not iterated over:\n"
> -				 "    key: %s\n  value: %s",
> -				 key_val[i][0], key_val[i][1]);
> -	}
> +	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
> +		cl_assert_equal_i(seen[i], 1);
>   
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> -	check_pointer_eq(get_test_entry(map, "notInMap", ignore_case), NULL);
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
> +	cl_assert_equal_p(get_test_entry(map, "notInMap", ignore_case), NULL);
>   }
>   
>   static void t_remove(struct hashmap *map, unsigned int ignore_case)
> @@ -211,24 +189,25 @@ static void t_remove(struct hashmap *map, unsigned int ignore_case)
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
>   		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   	}
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(remove); i++) {
>   		entry = alloc_test_entry(remove[i][0], "", ignore_case);
>   		removed = hashmap_remove_entry(map, entry, ent, remove[i][0]);
> -		if (check(removed != NULL))
> -			check_str(get_value(removed), remove[i][1]);
> +		cl_assert(removed != NULL);
> +		cl_assert_equal_s(get_value(removed), remove[i][1]);
>   		free(entry);
>   		free(removed);
>   	}
>   
>   	entry = alloc_test_entry("notInMap", "", ignore_case);
> -	check_pointer_eq(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
> +	cl_assert_equal_p(hashmap_remove_entry(map, entry, ent, "notInMap"), NULL);
>   	free(entry);
>   
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map),
> +			  ARRAY_SIZE(key_val) - ARRAY_SIZE(remove));
>   }
>   
>   static void t_iterate(struct hashmap *map, unsigned int ignore_case)
> @@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
>   
>   	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
>   		entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   	}
>   
>   	hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
>   	{
> -		int ret;
> -		if (!check_int((ret = key_val_contains(key_val, seen,
> -						       ARRAY_SIZE(key_val),
> -						       entry)), ==, 0)) {
> -			switch (ret) {
> -			case 1:
> -				test_msg("found entry was not given in the input\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			case 2:
> -				test_msg("duplicate entry detected\n"
> -					 "    key: %s\n  value: %s",
> -					 entry->key, get_value(entry));
> -				break;
> -			}
> -		}
> +		int ret = key_val_contains(key_val, seen,
> +					   ARRAY_SIZE(key_val),
> +					   entry);
> +		cl_assert(ret == 0);
>   	}
>   
> -	for (size_t i = 0; i < ARRAY_SIZE(seen); i++) {
> -		if (!check_int(seen[i], ==, 1))
> -			test_msg("following key-val pair was not iterated over:\n"
> -				 "    key: %s\n  value: %s",
> -				 key_val[i][0], key_val[i][1]);
> -	}
> +	for (size_t i = 0; i < ARRAY_SIZE(seen); i++)
> +		cl_assert_equal_i(seen[i], 1);
>   
> -	check_int(hashmap_get_size(map), ==, ARRAY_SIZE(key_val));
> +	cl_assert_equal_i(hashmap_get_size(map), ARRAY_SIZE(key_val));
>   }
>   
>   static void t_alloc(struct hashmap *map, unsigned int ignore_case)
> @@ -284,17 +246,17 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
>   		char *key = xstrfmt("key%d", i);
>   		char *value = xstrfmt("value%d", i);
>   		entry = alloc_test_entry(key, value, ignore_case);
> -		check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> +		cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>   		free(key);
>   		free(value);
>   	}
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, 51);
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), 51);
>   
>   	entry = alloc_test_entry("key52", "value52", ignore_case);
> -	check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> -	check_int(map->tablesize, ==, 256);
> -	check_int(hashmap_get_size(map), ==, 52);
> +	cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
> +	cl_assert_equal_i(map->tablesize, 256);
> +	cl_assert_equal_i(hashmap_get_size(map), 52);
>   
>   	for (int i = 1; i <= 12; i++) {
>   		char *key = xstrfmt("key%d", i);
> @@ -302,27 +264,27 @@ static void t_alloc(struct hashmap *map, unsigned int ignore_case)
>   
>   		entry = alloc_test_entry(key, "", ignore_case);
>   		removed = hashmap_remove_entry(map, entry, ent, key);
> -		if (check(removed != NULL))
> -			check_str(value, get_value(removed));
> +		cl_assert(removed != NULL);
> +		cl_assert_equal_s(value, get_value(removed));
>   		free(key);
>   		free(value);
>   		free(entry);
>   		free(removed);
>   	}
> -	check_int(map->tablesize, ==, 256);
> -	check_int(hashmap_get_size(map), ==, 40);
> +	cl_assert_equal_i(map->tablesize, 256);
> +	cl_assert_equal_i(hashmap_get_size(map), 40);
>   
>   	entry = alloc_test_entry("key40", "", ignore_case);
>   	removed = hashmap_remove_entry(map, entry, ent, "key40");
> -	if (check(removed != NULL))
> -		check_str("value40", get_value(removed));
> -	check_int(map->tablesize, ==, 64);
> -	check_int(hashmap_get_size(map), ==, 39);
> +	cl_assert(removed != NULL);
> +	cl_assert_equal_s("value40", get_value(removed));
> +	cl_assert_equal_i(map->tablesize, 64);
> +	cl_assert_equal_i(hashmap_get_size(map), 39);
>   	free(entry);
>   	free(removed);
>   }
>   
> -static void t_intern(void)
> +void test_hashmap__intern(void)
>   {
>   	const char *values[] = { "value1", "Value1", "value2", "value2" };
>   
> @@ -330,32 +292,68 @@ static void t_intern(void)
>   		const char *i1 = strintern(values[i]);
>   		const char *i2 = strintern(values[i]);
>   
> -		if (!check(!strcmp(i1, values[i])))
> -			test_msg("strintern(%s) returns %s\n", values[i], i1);
> -		else if (!check(i1 != values[i]))
> -			test_msg("strintern(%s) returns input pointer\n",
> -				 values[i]);
> -		else if (!check_pointer_eq(i1, i2))
> -			test_msg("address('%s') != address('%s'), so strintern('%s') != strintern('%s')",
> -				 i1, i2, values[i], values[i]);
> -		else
> -			check_str(i1, values[i]);
> +		cl_assert_equal_s(i1, values[i]);
> +		cl_assert(i1 != values[i]);
> +		cl_assert_equal_p(i1, i2);
>   	}
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_hashmap__replace_case_sensitive(void)
> +{
> +	setup(t_replace, 0);
> +}
> +
> +void test_hashmap__replace_case_insensitive(void)
> +{
> +	setup(t_replace, 1);
> +}
> +
> +void test_hashmap__get_case_sensitive(void)
> +{
> +	setup(t_get, 0);
> +}
> +
> +void test_hashmap__get_case_insensitive(void)
> +{
> +	setup(t_get, 1);
> +}
> +
> +void test_hashmap__add_case_sensitive(void)
> +{
> +	setup(t_add, 0);
> +}
> +
> +void test_hashmap__add_case_insensitive(void)
> +{
> +	setup(t_add, 1);
> +}
> +
> +void test_hashmap__remove_case_sensitive(void)
> +{
> +	setup(t_remove, 0);
> +}
> +
> +void test_hashmap__remove_case_insensitive(void)
> +{
> +	setup(t_remove, 1);
> +}
> +
> +void test_hashmap__iterate_case_sensitive(void)
> +{
> +	setup(t_iterate, 0);
> +}
> +
> +void test_hashmap__iterate_case_insensitive(void)
> +{
> +	setup(t_iterate, 1);
> +}
> +
> +void test_hashmap__alloc_case_sensitive(void)
> +{
> +	setup(t_alloc, 0);
> +}
> +
> +void test_hashmap__alloc_case_insensitive(void)
>   {
> -	TEST(setup(t_replace, 0), "replace works");
> -	TEST(setup(t_replace, 1), "replace (case insensitive) works");
> -	TEST(setup(t_get, 0), "get works");
> -	TEST(setup(t_get, 1), "get (case insensitive) works");
> -	TEST(setup(t_add, 0), "add works");
> -	TEST(setup(t_add, 1), "add (case insensitive) works");
> -	TEST(setup(t_remove, 0), "remove works");
> -	TEST(setup(t_remove, 1), "remove (case insensitive) works");
> -	TEST(setup(t_iterate, 0), "iterate works");
> -	TEST(setup(t_iterate, 1), "iterate (case insensitive) works");
> -	TEST(setup(t_alloc, 0), "grow / shrink works");
> -	TEST(t_intern(), "string interning works");
> -	return test_done();
> +	setup(t_alloc, 1);
>   }
Previous: Junio C HamanoNext: Patrick Steinhardt
Message 14 of 21 in “t/unit-tests: convert unit-tests to use clar”
  1. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 30, 2025
  2. 1/4 t/unit-tests: convert hashmap test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  3. Patrick SteinhardtJan 31, 2025
  4. 2/4 t/unit-tests: adapt example decorate test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  5. Patrick SteinhardtJan 31, 2025
  6. 3/4 t/unit-tests: convert strbuf test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  7. Patrick SteinhardtJan 31, 2025
  8. 4/4 t/unit-tests: convert strcmp-offset test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  9. Patrick SteinhardtJan 31, 2025
  10. Patrick SteinhardtJan 31, 2025
  11. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 31, 2025
  12. 1/4 t/unit-tests: convert hashmap test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  13. Junio C HamanoJan 31, 2025
  14. phillip.wood123@gmail.comFeb 2, 2025
  15. Patrick SteinhardtFeb 3, 2025
  16. phillip.wood123@gmail.comFeb 3, 2025
  17. 2/4 t/unit-tests: adapt example decorate test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  18. 3/4 t/unit-tests: convert strbuf test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  19. Phillip WoodFeb 2, 2025
  20. 4/4 t/unit-tests: convert strcmp-offset test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  21. Junio C HamanoJan 31, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.