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

> On Wed, May 29, 2024 at 11:41 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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.