From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 02 Jul 2024 15:24:54 GMT
Subject: Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c
Message-ID: <16e06a6d-5fd0-4132-9d82-5c6f13b7f9ed@gmail.com>
In-Reply-To: <20240628122030.41554-1-shyamthakkar001@gmail.com>

Hi Ghanshyam

On 28/06/2024 13:20, Ghanshyam Thakkar wrote:
> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h
> library which is built on top of hashmap.h.
> 
> Migrate them to the unit testing framework for better performance,
> concise code and better debugging. Along with the migration also plug
> memory leaks and make the test logic independent for all the tests.
> The migration removes 'put' tests from t0016, because it is used as
> setup to all the other tests, so testing it separately does not yield
> any benefit.
> 
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> This version addresses Phillip's review about detecting duplicates in
> oidmap when iterating over it and removing put_and_check_null() to move
> the relevant code to setup() instead. And contains some grammer fixes
> in the comment.

This version with Junio's fixup addresses my previous comments. One more 
thing occurred to me as I was reading it again

> +static void t_iterate(struct oidmap *map)
> +{
> +	struct oidmap_iter iter;
> +	struct test_entry *entry;

I wonder if we want to add a bit of paranoia with

	int count = 0;

> +	oidmap_iter_init(map, &iter);
> +	while ((entry = oidmap_iter_next(&iter))) {
> +		int ret;
> +		if (!check_int((ret = key_val_contains(entry)), ==, 0)) {
> +			switch (ret) {
> +			case -1:
> +				break; /* error message handled by get_oid_arbitrary_hex() */
> +			case 1:
> +				test_msg("obtained entry was not given in the input\n"
> +					 "  name: %s\n   oid: %s\n",
> +					 entry->name, oid_to_hex(&entry->entry.oid));
> +				break;
> +			case 2:
> +				test_msg("duplicate entry detected\n"
> +					 "  name: %s\n   oid: %s\n",
> +					 entry->name, oid_to_hex(&entry->entry.oid));
> +				break;
> +			default:
> +				test_msg("BUG: invalid return value (%d) from key_val_contains()",
> +					 ret);
> +				break;
> +			}
> +		} 
		} else {
			count++;
		}
> +	}
	check_int(count, ARRAY_SIZE(key_val));

to check that we iterate over all the entries as well as checking the 
size of the hashmap here.

 > +	check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));

Best Wishes

Phillip

> +}
> +
> +int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +{
> +	TEST(setup(t_replace), "replace works");
> +	TEST(setup(t_get), "get works");
> +	TEST(setup(t_remove), "remove works");
> +	TEST(setup(t_iterate), "iterate works");
> +	return test_done();
> +}