From: Patrick Steinhardt <ps@pks•im>
To: Seyi Kuforiji <kuforiji98@gmail•com>
Cc: git@vger•kernel.org, phillip.wood@dunelm•org.uk
Subject: Re: [PATCH 1/4] t/unit-tests: convert hashmap test to clar framework
Date: Fri, 31 Jan 2025 12:43:53 +0100 [thread overview]
Message-ID: <Z5y3ebBpzYM0wUVG@pks.im> (raw)
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
:)
> 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.
> @@ -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.
> @@ -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.
> @@ -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
next prev parent reply other threads:[~2025-01-31 11:43 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-30 9:13 [PATCH 0/4] t/unit-tests: convert unit-tests to use clar Seyi Kuforiji
2025-01-30 9:13 ` [PATCH 1/4] t/unit-tests: convert hashmap test to clar framework Seyi Kuforiji
2025-01-31 11:43 ` Patrick Steinhardt [this message]
2025-01-30 9:13 ` [PATCH 2/4] t/unit-tests: adapt example decorate " Seyi Kuforiji
2025-01-31 11:43 ` Patrick Steinhardt
2025-01-30 9:13 ` [PATCH 3/4] t/unit-tests: convert strbuf " Seyi Kuforiji
2025-01-31 11:43 ` Patrick Steinhardt
2025-01-30 9:13 ` [PATCH 4/4] t/unit-tests: convert strcmp-offset " Seyi Kuforiji
2025-01-31 11:44 ` Patrick Steinhardt
2025-01-31 11:43 ` [PATCH 0/4] t/unit-tests: convert unit-tests to use clar Patrick Steinhardt
2025-01-31 22:14 ` [PATCH v2 " Seyi Kuforiji
2025-01-31 22:14 ` [PATCH v2 1/4] t/unit-tests: convert hashmap test to use clar test framework Seyi Kuforiji
2025-01-31 23:03 ` Junio C Hamano
2025-02-02 11:09 ` phillip.wood123
2025-02-03 7:30 ` Patrick Steinhardt
2025-02-03 14:56 ` phillip.wood123
2025-01-31 22:14 ` [PATCH v2 2/4] t/unit-tests: adapt example decorate " Seyi Kuforiji
2025-01-31 22:14 ` [PATCH v2 3/4] t/unit-tests: convert strbuf " Seyi Kuforiji
2025-02-02 14:38 ` Phillip Wood
2025-01-31 22:14 ` [PATCH v2 4/4] t/unit-tests: convert strcmp-offset " Seyi Kuforiji
2025-01-31 23:06 ` [PATCH v2 0/4] t/unit-tests: convert unit-tests to use clar Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z5y3ebBpzYM0wUVG@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=kuforiji98@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox