Re: [PATCH 2/5] t/unit-tests: convert oid-array test to use clar

> On Thu, 20 Feb 2025 at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> 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 clear.
>>
> 
> I understand your concern about losing debugging information, but it
> is more beneficial to prioritize clarity and simplicity in unit tests.
> Unit tests should be short and easy, and adding extra debugging
> messages increases complexity, making them harder to maintain and
> read. The assertion failures already indicate where an issue occurs,
> allowing whoever is debugging to inspect the test inputs directly if
> needed.

> The lack of explicit debugging output is not a
> significant issue in practice. Furthermore, there are plans to
> collaborate with Clar upstream to equip common assertions with the
> ability to print custom messages in a formatted string where an error
> occurs. This would allow our test to be simple and easy to read and
> also maintain some of our custom debug messages.

> 
> 
>>>        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.
>>
> 
> Thanks for spotting this! Will fix this in an updated patch.
>> 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);
>>>    }
>>
> 
> Thanks
> Seyi