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

From: Patrick Steinhardt <[email-masked]>
Date: Fri, 2 May 2025 11:57:33 +0200

On Tue, Apr 29, 2025 at 04:04:22PM -0700, Junio C Hamano wrote:
> Seyi Kuforiji <[email-masked]> writes:
> > Signed-off-by: Seyi Kuforiji <[email-masked]>
> > ---
> >  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