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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Feb 21, 2025, 14:48 UTC
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:

Show 10 quoted lines

> 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.

Show 10 quoted lines

> -	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