Re: [GSoC][PATCH v2] t: port helper/test-hashmap.c to unit-tests/t-hashmap.c

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 09/07/2024 20:34, Josh Steadmon wrote:
> > On 2024.07.08 21:45, Ghanshyam Thakkar wrote:
>
> >> +static void t_put(struct hashmap *map, int ignore_case)
> >> +{
> >> +	struct test_entry *entry;
> >> +	const char *key_val[][2] = { { "key1", "value1" },
> >> +				     { "key2", "value2" },
> >> +				     { "fooBarFrotz", "value3" } };
> >> +
> >> +	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
> >> +		entry = alloc_test_entry(ignore_case, key_val[i][0], key_val[i][1]);
> >> +		check(hashmap_put_entry(map, entry, ent) == NULL);
> >> +	}
> >> +
> >> +	entry = alloc_test_entry(ignore_case, "foobarfrotz", "value4");
> >> +	entry = hashmap_put_entry(map, entry, ent);
> >> +	check(ignore_case ? entry != NULL : entry == NULL);
> >> +	free(entry);
> >> +
> >> +	check_int(map->tablesize, ==, 64);
> >> +	check_int(hashmap_get_size(map), ==,
> >> +		  ignore_case ? ARRAY_SIZE(key_val) : ARRAY_SIZE(key_val) + 1);
> >> +}
> > 
> > Ahhh, so you're using the same function for both case-sensitive and
> > -insensitive tests. So I guess TEST_RUN isn't useful here after all.
> > Personally I'd still rather get rid of setup(), but I don't feel super
> > strongly about it.
>
> I'm not sure - we have to pass ignore_case to HASHMAP_INIT and the test
> function so using setup() means we cannot pass different values to the
> two different functions.
>
> For parameterized tests where we calling the same function with
> different inputs using a setup function allows us to
> - write more concise test code
> - easily change the setup of all the tests if the api changes in the
> future.
> - consistently free resources at the end of a test making it easier to
> write leak-free tests
> - assert pre- and post- conditions on all tests
>
> Using TEST_RUN is useful to declare the different inputs for each test.
> For example in the oid-array tests it allows us to write
>
> TEST_RUN("ordered enumeration") {
> const char *input[] = { "88", "44", "aa", "55" };
> const char *expected[] = { "44", "55", "88", "aa" };
>
> TEST_ENUMERATION(input, expected)
> }
>
> rather than declaring a bunch of variables up front a long way from
> where they are used.

Yeah, I changed my mind from removing setup(). I'll keep it as it does
not have anything complex to make manual verifying harder. Also, since
this test does not have any variable inputs, we can just use TEST(),
although TEST_RUN() with variable inputs is very useful in other tests.

> >> +static void t_add(struct hashmap *map, int ignore_case)
> >> +{
> >> +	struct test_entry *entry;
> >> +	const char *key_val[][3] = {
> >> +		{ "key1", "value1", "UNUSED" },
> >> +		{ ignore_case ? "Key1" : "key1", "value2", "UNUSED" },
> >> +		{ "fooBarFrotz", "value3", "UNUSED" },
> >> +		{ ignore_case ? "Foobarfrotz" : "fooBarFrotz", "value4", "UNUSED" }
> >> +	};
> >> +	const char *queries[] = { "key1",
> >> +				  ignore_case ? "Foobarfrotz" : "fooBarFrotz" };
> >> +
> >> +	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
> >> +		entry = alloc_test_entry(ignore_case, key_val[i][0], key_val[i][1]);
> >> +		hashmap_add(map, &entry->ent);
> >> +	}
> >> +
> >> +	for (size_t i = 0; i < ARRAY_SIZE(queries); i++) {
> > 
> > Since we only have one query, can we remove the loop and simplify the
> > following block of code?
> > 
> > Also (here and elsewhere), it might be less confusing to say "UNSEEN" /
> > "SEEN" instead of "UNUSED" / "USED". The latter makes it sound to me
> > like there's some API requirement to have a 3-item array that we don't
> > actually need, but in this case those fields are actually used in
> > key_val_contains() to track duplicates.
>
> The third element just needs to be a boolean flag so it might be better
> to use a struct
>
> const struct {
> char *key;
> char *val;
> char seen;
> } key_val[] = {
> { .key = "key1", .val = "value1" },
> { .key = ignore_case ? "Key1" : "key1" .val = "value2" }, { .key =
> "fooBarFrotz" .val = "value3" },
> { .key = ignore_case ? "Foobarfrotz" : "fooBarFrotz", .value = "value4"
> }
> };

I think we can just use an extra 'char seen[]' for markings and modify
key_val_contains() to have this parameter, instead of having it as a
struct.

Thanks.