Re: [PATCH 6/6] t-strbuf: use TEST_RUN

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

Josh Steadmon <steadmon@google.com> wrote:
> On 2024.07.02 22:59, Ghanshyam Thakkar wrote:
> > Josh Steadmon <steadmon@google.com> wrote:
> > > I think this commit in particular shows how TEST_RUN() is more
> > > convenient than TEST(). (Although, arguably we shouldn't have allowed
> > > the setup() + callback situation to start with.)
> > 
> > Could you expand a bit on why the setup() + callback thing shouldn't be
> > allowed? I think it is a nice way of avoiding boilerplate and having
> > independent state.
>
> It's a matter of taste rather than strict principles, I suppose, but I
> think that test cases should focus on being readable rather than
> avoiding duplication. Production code can follow a Don't Repeat Yourself
> philosophy, but when a test breaks, it's better to be able to look at
> only the failing test and quickly see what's wrong, rather than having
> to work out which functions call which callbacks and what gets verified
> where. Also, since we don't have tests for our tests, it's important
> that the tests are easy for people to manually verify them.

Makes sense. But the manner in which callbacks are used in these tests,
which usually only initialize a certain datastructure and then release them at
the end, I fail to see them as more harder to manually verify. But it is
very easy to stretch the line and perhaps do more than initialization in
the setup(), so I can see how disallowing them would get rid of the need of
judging every time wether they are readable or not. Maybe a 'best
practices' document would help in that regard? 

> I also agree with René's point about the test cases matching real-world
> API usage.
>
> I do agree that having separate functions improves isolation, but
> hopefully we can encourage test authors to define local variables in
> their TEST_RUN blocks.

Makes sense.

Thanks.