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

> 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.

> >> +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"
> }
> };