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

[Junio C Hamano <gitster@pobox.com>]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

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

Hmph.  You seem to overwrite key_val[i][1] ...

> +/*
> + * Elements we will put in oidmap structs are made of a key: the entry.oid
> + * field, which is of type struct object_id, and a value: the name field (could
> + * be a refname for example).
> + */
> +struct test_entry {
> +	struct oidmap_entry entry;
> +	char name[FLEX_ARRAY];
> +};
> +
> +static const char *key_val[][2] = { { "11", "one" },
> +				    { "22", "two" },
> +				    { "33", "three" } };

... in this test, rendering the key_val[] array unusuable for
further tests.  Is that intended and desirable?

As long as t_iterate() stays to be the last test, that would be OK,
but once somebody wants to add a new test after it, i.e.

	TEST(setup(t_iterate), "iterate works");
+	TEST(setup(t_frotz), "frotz works");
	return test_done();

the setup() for that new test depends on the key_val[] array, whose
value fields key_val[i][1] have been modified.

The TEST(setup(t_foo)) pattern is done so nicely to make sure that
everybody is independent from everybody else, preparing the oidmap
used for each specific test from scratch.  It is a bit disappointing
that we are now invalidating this nice property.

> +static int key_val_contains(struct test_entry *entry)
> +{
> +	for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {
> +		struct object_id oid;
> +
> +		if (get_oid_arbitrary_hex(key_val[i][0], &oid))
> +			return -1;
> +
> +		if (oideq(&entry->entry.oid, &oid)) {
> +			if (!strcmp(key_val[i][1], "USED"))
> +				return 2;
> +			key_val[i][1] = "USED";
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}

Otherwise, the other tests looked reasonable.  Looking really nice.

As I expect things to be slow this week, being a big vacation week
in the US, we may not see much review activities this week.  I'll
queue this version in the meantime, not merging it down to 'next',
in case people start comment on it next week.

Thanks.

> +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();
> +}