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

> On 2024.05.30 08:54, Junio C Hamano wrote:
> > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > 
> > > The latter provides much more context (we almost don't have to open
> > > t-example-decorate.c file itself in some cases to know what failed)
> > > than the former. Now, of course we can add more test_msg()s to the
> > > former to improve, but I feel that this approach of splitting them
> > > provides and improves the information provided on stdout _without_
> > > adding any of my own test_msg()s. And I think that this is a good
> > > middleground between cluttering the stdout vs providing very little
> > > context while also remaining a faithful copy of the original.
> > 
> > If so, why stop at having four, each of which has more than one step
> > that could further be split?  What's the downside?
> > 
> >     Note: Here in this review, I am not necessarily suggesting the
> >     tests in this patch to be further split into greater number of
> >     smaller helper functions.  I am primarily interested in finding
> >     out what the unit test framework can further do to help unit
> >     tests written using it (i.e., like this patch).  If using
> >     finer-grained tests gives you better diagnosis, but if it is too
> >     cumbersome to separate the tests out further, is it because the
> >     framework is inadequate in some way?  How can we improve it?
> 
> I'll try not to speak for anyone else here, but I think the test
> framework isn't causing much friction here in the decision of how to
> split the tests. [However, neither is it providing much guidance. At
> some point we should review the unit tests and see if we can extract a
> helpful style guide or best practices doc.] The setup for the cases is
> minimal and done through the main function.

> I think the current split is reasonable as a first patch, as it mirrors
> the organization of the original test and makes it easier for reviewers
> to verify that it tests the same behaviors. If further simplification or
> reorganization is needed, I would like to see that as a separate patch
> on top of the more straightforward conversion.
> 
> The only part that bothers me a bit (and this is really more of a
> complaint about the framework than the patch itself) is the carryover of
> state between the different TEST() cases. We can't skip t_add and expect
> the other test cases to still pass, unfortunately. However, I don't
> think this patch needs to worry about that, since the framework doesn't
> restrict persistent state. [And we certainly don't restrict persistent
> state in the shell tests either.]