diff mbox series

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

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

Commit Message

Seyi Kuforiji Feb. 20, 2025, 8:29 a.m. UTC
Adapt oidmap test script to clar framework by using clar assertions
where necessary. `cl_parse_any_oid` handles the necessary checks needed
for the test to run smoothly.

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

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

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

Comments

Phillip Wood Feb. 21, 2025, 10:04 a.m. UTC | #1
Hi Seyi

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

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

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

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

Everything up to this point looks good.

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

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

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

Best Wishes

Phillip
diff mbox series

Patch

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