From: phillip.wood123@gmail.com
Date: Fri, 21 Feb 2025 14:48:26 GMT
Subject: Re: [PATCH 4/5] t/unit-tests: convert oidtree test to use clar
Message-ID: <2d60f5ae-7985-4400-a4d1-0931dac0653e@gmail.com>
In-Reply-To: <20250220082959.10854-5-kuforiji98@gmail.com>

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