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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Sep 4, 2024, 09:31 UTC
Message-ID: <33243592-1342-4a11-a592-63b35b0abe9d@gmail.com>
In-Reply-To: <ZtgAJFSXn2M252ZY@pks.im>

On 04/09/2024 07:37, Patrick Steinhardt wrote:

Show 20 quoted lines

> On Tue, Sep 03, 2024 at 10:48:01AM +0100, phillip.wood123@gmail.com wrote:
>> On 03/09/2024 08:45, Patrick Steinhardt wrote:
>>> On Wed, Aug 28, 2024 at 02:17:05PM +0100, Phillip Wood wrote:
>>>> 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,

Oh, I see - I'd misunderstood what you meant by "system under test"

Show 6 quoted lines

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

Isn't the point of strvec_init() to ensure that the `struct strvec` passed to it is usable? If it does not do that then having all the tests fail is good as it tells us there is a bug. I was hoping that __initialize() would be a useful replacement for the

	TEST(setup(test_fn()), "description")

pattern in some of our existing tests. I find hoisting the common initialization and cleanup out of each tests makes them easier to review as I can review the setup code once and not worry about it when reviewing each test. It also avoids a lot of repetition when writing tests and keeps the test bodies concise.

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

That's certainly a good use for them

Best Wishes

Phillip

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