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