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

From: Patrick Steinhardt <ps@pks.im>
Date: May 2, 2025, 09:57 UTC
Message-ID: <aBSXJcT8REHWrsrx@pks.im>
In-Reply-To: <20250429175302.23724-11-kuforiji98@gmail.com>

On Tue, Apr 29, 2025 at 06:53:02PM +0100, Seyi Kuforiji wrote:

Show 41 quoted lines

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

Show 23 quoted lines

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

Show 20 quoted lines

> 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