Re: [PATCH v2 01/10] t/unit-tests: implement reftable test helper functions in unit-test.{c,h}

From: Patrick Steinhardt <ps@pks.im>
Date: May 2, 2025, 09:57 UTC
Message-ID: <aBSXDQ5BjUfG-GKZ@pks.im>
In-Reply-To: <xmqqbjse4ljd.fsf@gitster.g>

On Tue, Apr 29, 2025 at 04:04:22PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Seyi Kuforiji <kuforiji98@gmail.com> writes:
> > Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> > ---
> >  t/unit-tests/unit-test.c | 93 ++++++++++++++++++++++++++++++++++++++++
> >  t/unit-tests/unit-test.h | 16 +++++++
> >  2 files changed, 109 insertions(+)
> 
> Hmph, this probably is a question better asked to Patrick, but it
> somehow feels a bit unsatisfactory that we are duplicating instead
> of replacing, as we cannot see "ah, this removed thing is now added
> in a different shape to fit in the other framework" in the patch---
> instead what we see in the patch is a bunch of "a completely new
> thing that honors the convention we are familiar in existing clar
> based tests is added here".

Well, my expectation is that at the end of this patch series we will remove the old functionality. The steps should roughly be:

  1. Implement equivalents for the reftable-specific test library for
     clar.

  2. Convert all reftable unit tests to use the new functions.

  3. Remove the old functions.

Given that (2) is a bunch of tests it makes perfect sense to first have a separate patch that adds without removing yet.

What I found a bit confusing though is that the new functions aren't added to "t/unit-tests/lib-reftable.{c,h}". This would be very much preferable from my point of view as the test functions aren't globally relevant, but really only relevant to the reftable library.

Patrick