diff mbox series

[4/5] t/unit-tests: convert oidtree test to use clar

Message ID 20250220082959.10854-5-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 oidtree 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_oidtree__initialize` handles the to set up of the global
oidtree variable and `test_oidtree__cleanup` frees the oidtree 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-oidtree.c => u-oidtree.c} | 78 +++++++++--------------
 3 files changed, 32 insertions(+), 50 deletions(-)
 rename t/unit-tests/{t-oidtree.c => u-oidtree.c} (44%)

Comments

Phillip Wood Feb. 21, 2025, 2:48 p.m. UTC | #1
Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Adapt oidtree 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_oidtree__initialize` handles the to set up of the global
> oidtree variable and `test_oidtree__cleanup` frees the oidtree when all
> tests are completed.
> 
> This streamlines the test suite, making individual tests self-contained
> and reducing redundant code.

My comments on the commit message for the last patch apply here as well.

> -	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
> -					     &expected), ==, 0))
> -		; /* the data is bogus and cannot be used */
> -	else if (!check(oideq(oid, &expected)))
> -		test_msg("expected: %s\n       got: %s\n     query: %s",
> -			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
> +	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
>   
> +	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
> +			 &expected);

using cl_parse_any_oid() means that it is safe to dispense with the 
check on the return value because we now do that check in cl_parse_oid().

> +	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));

This is nice - we'll get a message containing the two oid's if they 
don't match. We should use the same trick in patch 2.

As with the last patch we now bail out after the first error, we should 
mention that in the commit message.

> -	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
> -		test_msg("error: could not find some 'object_id's for query ('%s')", query);
> +	cl_assert_equal_i(hex_iter.i, hex_iter.expected_hexes.nr);

Using cl_failf() here would enable us to keep the message so that if the 
test fails we can see which query it was that failed.

Apart from that last point this is looking good

Best Wishes

Phillip
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 58a6af1eb0..feb01702c7 100644
--- a/Makefile
+++ b/Makefile
@@ -1358,6 +1358,7 @@  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-oidtree
 CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
@@ -1368,7 +1369,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-oidtree
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
diff --git a/t/meson.build b/t/meson.build
index f9e0ae15df..0b412a7c16 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-oid-array.c',
   'unit-tests/u-oidmap.c',
+  'unit-tests/u-oidtree.c',
   'unit-tests/u-prio-queue.c',
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
@@ -50,7 +51,6 @@  clar_unit_tests = executable('unit-tests',
 test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
-  'unit-tests/t-oidtree.c',
   'unit-tests/t-reftable-basics.c',
   'unit-tests/t-reftable-block.c',
   'unit-tests/t-reftable-merged.c',
diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/u-oidtree.c
similarity index 44%
rename from t/unit-tests/t-oidtree.c
rename to t/unit-tests/u-oidtree.c
index a38754b066..de6f6bd292 100644
--- a/t/unit-tests/t-oidtree.c
+++ b/t/unit-tests/u-oidtree.c
@@ -1,10 +1,11 @@ 
-#include "test-lib.h"
-#include "lib-oid.h"
+#include "unit-test.h"
 #include "oidtree.h"
 #include "hash.h"
 #include "hex.h"
 #include "strvec.h"
 
+static struct oidtree ot;
+
 #define FILL_TREE(tree, ...)                                       \
 	do {                                                       \
 		const char *hexes[] = { __VA_ARGS__ };             \
@@ -16,8 +17,7 @@  static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)
 {
 	for (size_t i = 0; i < n; i++) {
 		struct object_id oid;
-		if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
-			return -1;
+		cl_parse_any_oid(hexes[i], &oid);
 		oidtree_insert(ot, &oid);
 	}
 	return 0;
@@ -27,10 +27,8 @@  static void check_contains(struct oidtree *ot, const char *hex, int expected)
 {
 	struct object_id oid;
 
-	if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
-		return;
-	if (!check_int(oidtree_contains(ot, &oid), ==, expected))
-		test_msg("oid: %s", oid_to_hex(&oid));
+	cl_parse_any_oid(hex, &oid);
+	cl_assert_equal_i(oidtree_contains(ot, &oid), expected);
 }
 
 struct expected_hex_iter {
@@ -44,19 +42,11 @@  static enum cb_next check_each_cb(const struct object_id *oid, void *data)
 	struct expected_hex_iter *hex_iter = data;
 	struct object_id expected;
 
-	if (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {
-		test_msg("error: extraneous callback for query: ('%s'), object_id: ('%s')",
-			 hex_iter->query, oid_to_hex(oid));
-		return CB_BREAK;
-	}
-
-	if (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],
-					     &expected), ==, 0))
-		; /* the data is bogus and cannot be used */
-	else if (!check(oideq(oid, &expected)))
-		test_msg("expected: %s\n       got: %s\n     query: %s",
-			 oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);
+	cl_assert(hex_iter->i < hex_iter->expected_hexes.nr);
 
+	cl_parse_any_oid(hex_iter->expected_hexes.v[hex_iter->i],
+			 &expected);
+	cl_assert_equal_s(oid_to_hex(oid), oid_to_hex(&expected));
 	hex_iter->i += 1;
 	return CB_CONTINUE;
 }
@@ -75,48 +65,40 @@  static void check_each(struct oidtree *ot, const char *query, ...)
 		strvec_push(&hex_iter.expected_hexes, arg);
 	va_end(hex_args);
 
-	if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
-		return;
+	cl_parse_any_oid(query, &oid);
 	oidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);
 
-	if (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))
-		test_msg("error: could not find some 'object_id's for query ('%s')", query);
+	cl_assert_equal_i(hex_iter.i, hex_iter.expected_hexes.nr);
 	strvec_clear(&hex_iter.expected_hexes);
 }
 
-static void setup(void (*f)(struct oidtree *ot))
+void test_oidtree__initialize(void)
 {
-	struct oidtree ot;
-
 	oidtree_init(&ot);
-	f(&ot);
-	oidtree_clear(&ot);
 }
 
-static void t_contains(struct oidtree *ot)
+void test_oidtree__cleanup(void)
 {
-	FILL_TREE(ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
-	check_contains(ot, "44", 0);
-	check_contains(ot, "441", 0);
-	check_contains(ot, "440", 0);
-	check_contains(ot, "444", 1);
-	check_contains(ot, "4440", 1);
-	check_contains(ot, "4444", 0);
+	oidtree_clear(&ot);
 }
 
-static void t_each(struct oidtree *ot)
+void test_oidtree__contains(void)
 {
-	FILL_TREE(ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
-	check_each(ot, "12300", "123", NULL);
-	check_each(ot, "3211", NULL); /* should not reach callback */
-	check_each(ot, "3210", "321", NULL);
-	check_each(ot, "32100", "321", NULL);
-	check_each(ot, "32", "320", "321", NULL);
+	FILL_TREE(&ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
+	check_contains(&ot, "44", 0);
+	check_contains(&ot, "441", 0);
+	check_contains(&ot, "440", 0);
+	check_contains(&ot, "444", 1);
+	check_contains(&ot, "4440", 1);
+	check_contains(&ot, "4444", 0);
 }
 
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
+void test_oidtree__each(void)
 {
-	TEST(setup(t_contains), "oidtree insert and contains works");
-	TEST(setup(t_each), "oidtree each works");
-	return test_done();
+	FILL_TREE(&ot, "f", "9", "8", "123", "321", "320", "a", "b", "c", "d", "e");
+	check_each(&ot, "12300", "123", NULL);
+	check_each(&ot, "3211", NULL); /* should not reach callback */
+	check_each(&ot, "3210", "321", NULL);
+	check_each(&ot, "32100", "321", NULL);
+	check_each(&ot, "32", "320", "321", NULL);
 }