Re: [PATCH v6 11/13] t/unit-tests: convert strvec tests to use clar

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 4, 2024, 06:37 UTC
Message-ID: <ZtgAJFSXn2M252ZY@pks.im>
In-Reply-To: <3be1d990-9f9c-4963-bce3-742511b79022@gmail.com>

On Tue, Sep 03, 2024 at 10:48:01AM +0100, phillip.wood123@gmail.com wrote:

Show 20 quoted lines

> Hi Patrick
> 
> On 03/09/2024 08:45, Patrick Steinhardt wrote:
> > On Wed, Aug 28, 2024 at 02:17:05PM +0100, Phillip Wood wrote:
> > > Hi Patrick
> > > 
> > > On 20/08/2024 15:02, Patrick Steinhardt wrote:
> > > It might be a good opportunity to show the set-up and tear-down facilities
> > > in clar as well instead of repeating the initialization in each test.
> > 
> > I don't think it's a good fit here, as setup and teardown would hit the
> > system under test. I rather think they should be used in cases where you
> > e.g. always have to setup a repository for your tests.
> 
> I'm not sure I follow. I was suggesting we define test_strvec__initialize()
> to initialize a global strvec which the tests use and is then freed by
> test_strvec__cleanup() like the tests/adding.c example the clar's README.md.
> That would allow use to remove the setup and teardown from each test. As I
> understand it clar's setup/cleanup functionality is usable without setting
> up a sandbox directory for each test.

What I'm saying is that `strvec_init()` itself is part of the system under test, so evicting that into a `__initialize()` function doesn't quite make sense to me. If there was for example a bug somewhere in the strvec code we might bring the global `struct strvec` into a state that is completely unusable, and thus all subsequent tests would fail. We could of course work around that by always zeroing out the struct, but because of that I just don't think it's a good fit.

I rather see the usefulness of `__initialize()` in setting up auxiliary data structures that are a dependency of the system under test, but which are not the system under test itself.

> I'll take a look at v7 in the next few days - I suspect we're getting to the
> point where it's ready to be merged.

Thanks!