Re: [PATCH v2 10/10] t/unit-tests: adapt lib-reftable{c,h} helper functions to clar

[Seyi Chamber <kuforiji98@gmail.com>]

Hi Patrick,
On Fri, 2 May 2025 at 10:58, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Apr 29, 2025 at 06:53:02PM +0100, Seyi Kuforiji wrote:
> > Helper functions defined in `t/unit-tests/lib-reftable.{c,h}` are
> > required for the reftable-related test files to run efficeintly. In the
> > current implementation these functions are designed to conform with our
> > homegrown unit-testing structure. So in other to convert the reftable
> > test files, there is need for a clar specific implementation of these
> > helper functions.
> >
> > type cast `for (size_t i = 0; i < (size_t)stats->ref_stats.blocks; i++)`
> > Adapt functions in lib-reftable.{c,h} to use clar. These functions
> > conform with the clar testing framework and become available for all
> > reftable-related test files implemented using the clar testing
> > framework, which requires them. This change migrates the helper
> > functions back into `lib-reftable.{c,h}`.
> >
> > Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> > ---
> >  Makefile                    |  4 +-
> >  t/meson.build               |  4 +-
> >  t/unit-tests/lib-reftable.c | 26 +++++------
> >  t/unit-tests/lib-reftable.h |  6 +--
> >  t/unit-tests/unit-test.c    | 93 -------------------------------------
> >  t/unit-tests/unit-test.h    | 16 -------
> >  6 files changed, 20 insertions(+), 129 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 0b42893611..7e646e16ee 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1379,12 +1379,12 @@ CLAR_TEST_SUITES += u-urlmatch-normalization
> >  CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
> >  CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
> >  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
> > -CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
> >  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
> > +CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
> > +CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
> >
> >  UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> >  UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> > -UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
> >
>
> `UNIT_TEST_PROGS` only contains the "unit-test" binary now, right? Does
> this mean that we can simplify build rules in our Makefile now?
>
> > diff --git a/t/meson.build b/t/meson.build
> > index 8fa00fc9ef..7c305a90b5 100644
> > --- a/t/meson.build
> > +++ b/t/meson.build
> > @@ -26,8 +26,9 @@ clar_test_suites = [
> >
> >  clar_sources = [
> >    'unit-tests/clar/clar.c',
> > +  'unit-tests/lib-oid.c',
> > +  'unit-tests/lib-reftable.c',
> >    'unit-tests/unit-test.c',
> > -  'unit-tests/lib-oid.c'
> >  ]
> >
> >  clar_decls_h = custom_target(
> > @@ -69,7 +70,6 @@ foreach unit_test_program : unit_test_programs
> >    unit_test = executable(unit_test_name,
> >      sources: [
> >        'unit-tests/test-lib.c',
> > -      'unit-tests/lib-reftable.c',
> >        unit_test_program,
> >      ],
> >      dependencies: [libgit_commonmain],
>
> Can't we remove this completely? `unit_test_programs` is empty now, so
> this loop is a no-op now.
>
> > diff --git a/t/unit-tests/lib-reftable.h b/t/unit-tests/lib-reftable.h
> > index e4c360fa7e..2958db5dc0 100644
> > --- a/t/unit-tests/lib-reftable.h
> > +++ b/t/unit-tests/lib-reftable.h
> > @@ -6,12 +6,12 @@
> >
> >  struct reftable_buf;
> >
> > -void t_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);
> > +void cl_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);
> >
> > -struct reftable_writer *t_reftable_strbuf_writer(struct reftable_buf *buf,
> > +struct reftable_writer *cl_reftable_strbuf_writer(struct reftable_buf *buf,
> >                                                struct reftable_write_options *opts);
> >
> > -void t_reftable_write_to_buf(struct reftable_buf *buf,
> > +void cl_reftable_write_to_buf(struct reftable_buf *buf,
> >                            struct reftable_ref_record *refs,
> >                            size_t nrecords,
> >                            struct reftable_log_record *logs,
>
> It is quite weird that we declare the replacement functions in
> "unit-test.h" in the first commit only to remove them at a later point.
> It would make way more sense if we introduced the functions in
> "t/unit/lib-reftable.{c,h}" right from the start and then only remove
> the unused functions in the last step.
>
> Patrick

If I get it correctly, you're suggesting I have both the original
functions and the clar-based variant in `t/unit/lib-reftable.{c,h}`
right from the first commit, right?

Thanks
Seyi