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

[phillip.wood123@gmail.com]

Hi Seyi

On 24/02/2025 09:11, Seyi Chamber wrote:
> 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.

If the test failure is on a CI run for a platform that the person 
debugging the failure does not have access to how are they going to do 
that? This is not a hypothetical concern as our CI runs the test suite 
on MacOs, Linux and Windows. Individual developers often only have 
access to one or to of those platforms. My experience of debugging CI 
test failures is that without decent diagnostic messages it is extremely 
difficult to figure out what went wrong if one does not have access to 
the platform where the test is failing.

> Assertion failures are rarely hit in real-world scenarios, and when
> they do occur, one can manually print values or use a debugger to
> investigate. This makes the additional debugging messages unnecessary
> in most cases.

This seems to be arguing that because we expect the tests to pass we 
don't need to worry about how difficult it is to debug them when they 
fail. I do not agree with that line of argument.

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

Have you got any more details about this please? We already have 
cl_failf() in our codebase.

Best Wishes

Phillip

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