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

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Jul 8, 2024, 21:59 UTC
Message-ID: <D2KHTOXCNFOI.Y1UIN4IT8APB@gmail.com>
In-Reply-To: <tswyfparvchgi7qxrjxbx4eb7cohypzekjqzbnkbffsesaiazs@vtewtz7o6twi>

Josh Steadmon <steadmon@google.com> wrote:

Show 18 quoted lines

> 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?

Show 6 quoted lines

> 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.