On Wed, May 29, 2024 at 11:41 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes:
> > + 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?
The original code has some kind of "sections" (or paragraphs)
separated using comments like:
/*
* Add 2 objects, one with a non-NULL decoration and one with a NULL
* decoration.
*/
or:
/*
* When re-adding an already existing object, the old decoration is
* returned.
*/
I think it makes sense to separate the code using functions matching
these "sections" and to reuse each comment in the TEST() macro that
calls the corresponding function. If this patch is rerolled for some
reason, I think it would be a good idea to mention this in the commit
message though.
