From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 10 Jul 2024 10:19:31 GMT
Subject: Re: [GSoC][PATCH v2] t: port helper/test-hashmap.c to unit-tests/t-hashmap.c
Message-ID: <2d7fd61d-0b2c-465f-9f24-47c99ab5b56c@gmail.com>
In-Reply-To: <mlnerj7j6knamzj3ipnd7rgqd6xm5xrjep35rldhv6sikzipu5@72szgbso6cpo>

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

Best Wishes

Phillip