Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework

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

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> +struct test_vars {
> +	struct object *one, *two, *three;
> +	struct decoration n;
> +	int decoration_a, decoration_b;
> +};
> +
> +static void t_add(struct test_vars *vars)
> +{
> +	void *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);
> +
> +	if (!check(ret == NULL))
> +		test_msg("when adding a brand-new object, NULL should be returned");
> +	ret = add_decoration(&vars->n, vars->two, NULL);
> +	if (!check(ret == NULL))
> +		test_msg("when adding a brand-new object, NULL should be returned");
> +}
> +
> +static void t_readd(struct test_vars *vars)
> +{
> +	void *ret = add_decoration(&vars->n, vars->one, NULL);
> +
> +	if (!check(ret == &vars->decoration_a))
> +		test_msg("when readding an already existing object, existing decoration should be returned");
> +	ret = add_decoration(&vars->n, vars->two, &vars->decoration_b);
> +	if (!check(ret == NULL))
> +		test_msg("when readding an already existing object, existing decoration should be returned");
> +}
> +
> +static void t_lookup(struct test_vars *vars)
> +{
> +	void *ret = lookup_decoration(&vars->n, vars->one);
> +
> +	if (!check(ret == NULL))
> +		test_msg("lookup should return added declaration");
> +	ret = lookup_decoration(&vars->n, vars->two);
> +	if (!check(ret == &vars->decoration_b))
> +		test_msg("lookup should return added declaration");
> +	ret = lookup_decoration(&vars->n, vars->three);
> +	if (!check(ret == NULL))
> +		test_msg("lookup for unknown object should return NULL");
> +}
> +
> +static void t_loop(struct test_vars *vars)
> +{
> +	int i, objects_noticed = 0;
> +
> +	for (i = 0; i < vars->n.size; i++) {
> +		if (vars->n.entries[i].base)
> +			objects_noticed++;
> +	}
> +	if (!check_int(objects_noticed, ==, 2))
> +		test_msg("should have 2 objects");
> +}
> +
> +int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +{
> +	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
> +	struct test_vars vars = { 0 };
> +
> +	vars.one = lookup_unknown_object(the_repository, &one_oid);
> +	vars.two = lookup_unknown_object(the_repository, &two_oid);
> +	vars.three = lookup_unknown_object(the_repository, &three_oid);
> +
> +	TEST(t_add(&vars),
> +	     "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.");
> +	TEST(t_readd(&vars),
> +	     "When re-adding an already existing object, the old decoration is returned.");
> +	TEST(t_lookup(&vars),
> +	     "Lookup returns the added declarations, or NULL if the object was never added.");
> +	TEST(t_loop(&vars), "The user can also loop through all entries.");

These tests as a whole look like a faithful copy of the original
done by cmd__example_decorate().

I do not understand the criteria used to split them into the four
separate helper functions.  It is not like they can be reused or
reordered---for example, t_readd() must be done after t_add() has
been done.

What benefit are you trying to get out of these split?  IOW, what
are we gaining by having four separate helper functions, instead of
testing all of these same things in a single helper function t_all
with something like

	TEST(t_all(&vars), "Do all decorate tests.");

in cmd_main()?  If there is a concrete benefit of having larger
number of smaller tests, would it make the result even better if we
split t_add() further into t_add_one() that adds one with deco_a and
t_add_two() that adds two with NULL?  The other helpers can of
course be further split into individual pieces the same way.  What
ere the criteria used to decide where to stop and use these four?

Thanks.