Re: [PATCH 1/4] t/unit-tests: convert hashmap test to clar framework
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 31, 2025, 11:43 UTC
- Message-ID
- <Z5y3ebBpzYM0wUVG@pks.im>
- In-Reply-To
- <20250130091334.39922-2-kuforiji98@gmail.com>
On Thu, Jan 30, 2025 at 10:13:31AM +0100, Seyi Kuforiji wrote:
> Adapts hashmap test script to clar framework by using clar assertions
s/Adapts/Adapt as we generally want to use imperative wording in commit messages, as if we're instructing the code to change.
Also, please note that the code is not a script. Scripts get executed by an interpreter, but C files are compiled by a compiler before they can get executed. So you should be talking about "code", not "scripts".
> where necessary. Test functions are created as both standalone and > inline to test different test cases.
I honestly don't quite know what this second sentence is supposed to say :)
Show 26 quoted lines
> diff --git a/t/unit-tests/t-hashmap.c b/t/unit-tests/u-hashmap.c
> similarity index 60%
> rename from t/unit-tests/t-hashmap.c
> rename to t/unit-tests/u-hashmap.c
> index 83b79dff39..6b6d22005a 100644
> --- a/t/unit-tests/t-hashmap.c
> +++ b/t/unit-tests/u-hashmap.c
> @@ -1,4 +1,4 @@
> -#include "test-lib.h"
> +#include "unit-test.h"
> #include "hashmap.h"
> #include "strbuf.h"
>
> @@ -83,23 +83,23 @@ static void t_replace(struct hashmap *map, unsigned int ignore_case)
> struct test_entry *entry;
>
> entry = alloc_test_entry("key1", "value1", ignore_case);
> - check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> + cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
>
> entry = alloc_test_entry(ignore_case ? "Key1" : "key1", "value2",
> ignore_case);
> entry = hashmap_put_entry(map, entry, ent);
> - if (check(entry != NULL))
> - check_str(get_value(entry), "value1");
> + cl_assert(entry != NULL);We usually avoid explicit `!= NULL`, but I guess in this case it's fine to keep this as-is as you simply keep the preexisting style.
Show 27 quoted lines
> @@ -165,39 +163,19 @@ static void t_add(struct hashmap *map, unsigned int ignore_case)
>
> hashmap_for_each_entry_from(map, entry, ent)
> {
> - int ret;
> - if (!check_int((ret = key_val_contains(
> - key_val, seen,
> - ARRAY_SIZE(key_val), entry)),
> - ==, 0)) {
> - switch (ret) {
> - case 1:
> - test_msg("found entry was not given in the input\n"
> - " key: %s\n value: %s",
> - entry->key, get_value(entry));
> - break;
> - case 2:
> - test_msg("duplicate entry detected\n"
> - " key: %s\n value: %s",
> - entry->key, get_value(entry));
> - break;
> - }
> - } else {
> - count++;
> - }
> + int ret = key_val_contains(key_val, seen,
> + ARRAY_SIZE(key_val), entry);
> + cl_assert(ret == 0);This could instead use `cl_assert_equal_i(ret, 0)` so that the error message mentions what the observed error code is.
Show 30 quoted lines
> @@ -242,38 +221,21 @@ static void t_iterate(struct hashmap *map, unsigned int ignore_case)
>
> for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
> entry = alloc_test_entry(key_val[i][0], key_val[i][1], ignore_case);
> - check_pointer_eq(hashmap_put_entry(map, entry, ent), NULL);
> + cl_assert_equal_p(hashmap_put_entry(map, entry, ent), NULL);
> }
>
> hashmap_for_each_entry(map, &iter, entry, ent /* member name */)
> {
> - int ret;
> - if (!check_int((ret = key_val_contains(key_val, seen,
> - ARRAY_SIZE(key_val),
> - entry)), ==, 0)) {
> - switch (ret) {
> - case 1:
> - test_msg("found entry was not given in the input\n"
> - " key: %s\n value: %s",
> - entry->key, get_value(entry));
> - break;
> - case 2:
> - test_msg("duplicate entry detected\n"
> - " key: %s\n value: %s",
> - entry->key, get_value(entry));
> - break;
> - }
> - }
> + int ret = key_val_contains(key_val, seen,
> + ARRAY_SIZE(key_val),
> + entry);Indentation is off here.
Show 6 quoted lines
> @@ -330,32 +292,68 @@ static void t_intern(void)
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_hashmap__replace_case_sensitive(void)
> +{
> + setup(t_replace, 0);
> +}I was briefly wondering whether we should use `__initialize()` and `__teardown()` functions instead of this setup function so that we can then get rid of `t_replace()` and other test functions. But then I realized that we want ot have these separate functions anyway so that we can easily test with case-sensitivity enabled and disabled, so this is probably okay.
Patrick