From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 20 Feb 2025 14:38:30 GMT
Subject: Re: [PATCH 2/5] t/unit-tests: convert oid-array test to use clar
Message-ID: <cf9d44d0-20ed-4d8f-84a3-1bf284075605@gmail.com>
In-Reply-To: <20250220082959.10854-3-kuforiji98@gmail.com>

Hi Seyi

On 20/02/2025 08:29, Seyi Kuforiji wrote:
> Adapt oid-array test script to clar framework by using clar assertions
> where necessary. Remove descriptions from macros to reduce
> redundancy, and move test input arrays to global scope for reuse across
> multiple test functions. Introduce `test_oid_array__initialize()` to
> explicitly initialize the hash algorithm.
> 
> These changes streamline the test suite, making individual tests
> self-contained and reducing redundant code.

I think these conversion look correct but once again we're losing 
valuable debugging information because we haven't added better 
assertions to clar.

>   	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> -	if (!check_uint(actual.nr, ==, expect.nr))
> -		return;
> -
> -	for (i = 0; i < actual.nr; i++) {
> -		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
> -			test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
> -				 oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
> -				 (uintmax_t)i);
> -	}
> +	cl_assert_equal_i(actual.nr, expect.nr);
> +
> +	for (i = 0; i < actual.nr; i++)
> +		cl_assert(oideq(&actual.oid[i], &expect.oid[i]));

If this fails the poor person debugging it will have no idea why as 
there is now no indication of which two oids were being compared.

> -	if (!check_int(ret, <=, upper_bound) ||
> -	    !check_int(ret, >=, lower_bound))
> -		test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
> +	cl_assert(ret <= upper_bound);
> +	cl_assert(ret >= lower_bound);

This is another case where we could do with better assertions in clar

> -static void setup(void)
> +void test_oid_array__initialize(void)
>   {
>   	/* The hash algo is used by oid_array_lookup() internally */
>   	int algo = init_hash_algo();
> -	if (check_int(algo, !=, GIT_HASH_UNKNOWN))
> -		repo_set_hash_algo(the_repository, algo);
> +	cl_assert(algo != GIT_HASH_UNKNOWN);

init_has_algo() in unit-test.c already does this.

Best Wishes

Phillip


> +	repo_set_hash_algo(the_repository, algo);
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +static const char *arr_input[] = { "88", "44", "aa", "55" };
> +static const char *arr_input_dup[] = { "88", "44", "aa", "55",
> +				       "88", "44", "aa", "55",
> +				       "88", "44", "aa", "55" };
> +static const char *res_sorted[] = { "44", "55", "88", "aa" };
> +
> +void test_oid_array__enumerate_unique(void)
>   {
> -	const char *arr_input[] = { "88", "44", "aa", "55" };
> -	const char *arr_input_dup[] = { "88", "44", "aa", "55",
> -					"88", "44", "aa", "55",
> -					"88", "44", "aa", "55" };
> -	const char *res_sorted[] = { "44", "55", "88", "aa" };
> -	const char *nearly_55;
> +	TEST_ENUMERATION(arr_input, res_sorted);
> +}
> +
> +void test_oid_array__enumerate_duplicate(void)
> +{
> +	TEST_ENUMERATION(arr_input_dup, res_sorted);
> +}
> +
> +void test_oid_array__lookup(void)
> +{
> +	TEST_LOOKUP(arr_input, "55", 1, 1);
> +}
>   
> -	if (!TEST(setup(), "setup"))
> -		test_skip_all("hash algo initialization failed");
> +void test_oid_array__lookup_non_existent(void)
> +{
> +	TEST_LOOKUP(arr_input, "33", INT_MIN, -1);
> +}
> +
> +void test_oid_array__lookup_duplicates(void)
> +{
> +	TEST_LOOKUP(arr_input_dup, "55", 3, 5);
> +}
>   
> -	TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration");
> -	TEST_ENUMERATION(arr_input_dup, res_sorted,
> -			 "ordered enumeration with duplicate suppression");
> +void test_oid_array__lookup_non_existent_dup(void)
> +{
> +	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1);
> +}
>   
> -	TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
> -	TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
> -	TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
> -	TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
> -		    "lookup non-existent entry with duplicates");
> +void test_oid_array__lookup_almost_dup(void)
> +{
> +	const char *nearly_55;
>   
>   	nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
>   			"5500000000000000000000000000000000000001" :
>   			"5500000000000000000000000000000000000000000000000000000000000001";
> -	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
> -		    "lookup with almost duplicate values");
> -	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
> -		    "lookup with single duplicate value");
>   
> -	return test_done();
> +	TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0);
> +}
> +
> +void test_oid_array__lookup_single_dup(void)
> +{
> +	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1);
>   }