On Thu, 20 Feb 2025 at 15:38, Phillip Wood <phillip.wood123@xxxxxxxxx> 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.
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. 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
