patch, 11 partst: port reftable/record_test.c to the unit testing framework

> reftable/record_test.c exercises the functions defined in
> reftable/record.{c, h}. Migrate reftable/record_test.c to the
> unit testing framework. Migration involves refactoring the tests
> to use the unit testing framework instead of reftable's test
> framework.
> While at it, change the type of index variable 'i' to 'size_t'
> from 'int'. This is because 'i' is used in comparison against
> 'ARRAY_SIZE(x)' which is of type 'size_t'.
>
> Also, use set_hash() which is defined locally in the test file
> instead of set_test_hash() which is defined by
> reftable/test_framework.{c, h}. This is fine to do as both these
> functions are similarly implemented, and
> reftable/test_framework.{c, h} is not #included in the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  Makefile                                      |   2 +-
>  t/helper/test-reftable.c                      |   1 -
>  .../unit-tests/t-reftable-record.c            | 106 ++++++++----------
>  3 files changed, 50 insertions(+), 59 deletions(-)
>  rename reftable/record_test.c => t/unit-tests/t-reftable-record.c (77%)
>
> diff --git a/Makefile b/Makefile
> index f25b2e80a1..def3700b4d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash
>  UNIT_TEST_PROGRAMS += t-mem-pool
>  UNIT_TEST_PROGRAMS += t-prio-queue
>  UNIT_TEST_PROGRAMS += t-reftable-basics
> +UNIT_TEST_PROGRAMS += t-reftable-record
>  UNIT_TEST_PROGRAMS += t-strbuf
>  UNIT_TEST_PROGRAMS += t-strcmp-offset
>  UNIT_TEST_PROGRAMS += t-strvec
> @@ -2678,7 +2679,6 @@ REFTABLE_TEST_OBJS += reftable/block_test.o
>  REFTABLE_TEST_OBJS += reftable/dump.o
>  REFTABLE_TEST_OBJS += reftable/merged_test.o
>  REFTABLE_TEST_OBJS += reftable/pq_test.o
> -REFTABLE_TEST_OBJS += reftable/record_test.o
>  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
>  REFTABLE_TEST_OBJS += reftable/stack_test.o
>  REFTABLE_TEST_OBJS += reftable/test_framework.o
> diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> index 9160bc5da6..aa6538a8da 100644
> --- a/t/helper/test-reftable.c
> +++ b/t/helper/test-reftable.c
> @@ -5,7 +5,6 @@
>  int cmd__reftable(int argc, const char **argv)
>  {
>  	/* test from simple to complex. */
> -	record_test_main(argc, argv);
>  	block_test_main(argc, argv);
>  	tree_test_main(argc, argv);
>  	pq_test_main(argc, argv);
> diff --git a/reftable/record_test.c b/t/unit-tests/t-reftable-record.c
> similarity index 77%
> rename from reftable/record_test.c
> rename to t/unit-tests/t-reftable-record.c
> index 58290bdba3..1b357e6c7f 100644
> --- a/reftable/record_test.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -6,13 +6,9 @@
>    https://developers.google.com/open-source/licenses/bsd
>  */
>
> -#include "record.h"
> -
> -#include "system.h"
> -#include "basics.h"
> -#include "constants.h"
> -#include "test_framework.h"
> -#include "reftable-tests.h"
> +#include "test-lib.h"
> +#include "reftable/constants.h"
> +#include "reftable/record.h"
>
>  static void test_copy(struct reftable_record *rec)
>  {
> @@ -24,9 +20,9 @@ static void test_copy(struct reftable_record *rec)
>  	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>  	/* do it twice to catch memory leaks */

>  	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
> -	EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> +	check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>
> -	puts("testing print coverage:\n");
> +	test_msg("testing print coverage:");
>  	reftable_record_print(&copy, GIT_SHA1_RAWSZ);
>

>  	reftable_record_release(&copy);
> @@ -43,8 +39,8 @@ static void test_varint_roundtrip(void)
>  			      4096,
>  			      ((uint64_t)1 << 63),
>  			      ((uint64_t)1 << 63) + ((uint64_t)1 << 63) - 1 };
> -	int i = 0;
> -	for (i = 0; i < ARRAY_SIZE(inputs); i++) {
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(inputs); i++) {
>  		uint8_t dest[10];
>
>  		struct string_view out = {
> @@ -55,29 +51,26 @@ static void test_varint_roundtrip(void)
>  		int n = put_var_int(&out, in);
>  		uint64_t got = 0;
>
> -		EXPECT(n > 0);
> +		check_int(n, >, 0);
>  		out.len = n;
>  		n = get_var_int(&got, &out);
> -		EXPECT(n > 0);
> +		check_int(n, >, 0);
>
> -		EXPECT(got == in);
> +		check_int(got, ==, in);
>  	}
>  }
>
>  static void set_hash(uint8_t *h, int j)
>  {
> -	int i = 0;
> -	for (i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) {
> +	for (int i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++)
>  		h[i] = (j >> i) & 0xff;
> -	}
>  }
>
>  static void test_reftable_ref_record_roundtrip(void)
>  {
>  	struct strbuf scratch = STRBUF_INIT;
> -	int i = 0;
>
> -	for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
> +	for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
>  		struct reftable_record in = {
>  			.type = BLOCK_TYPE_REF,
>  		};
> @@ -109,17 +102,17 @@ static void test_reftable_ref_record_roundtrip(void)
>
>  		test_copy(&in);
>
> -		EXPECT(reftable_record_val_type(&in) == i);
> +		check_int(reftable_record_val_type(&in), ==, i);
>
>  		reftable_record_key(&in, &key);
>  		n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> -		EXPECT(n > 0);
> +		check_int(n, >, 0);
>
>  		/* decode into a non-zero reftable_record to test for leaks. */
>  		m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch);
> -		EXPECT(n == m);
> +		check_int(n, ==, m);
>
> -		EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
> +		check(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
>  						 GIT_SHA1_RAWSZ));
>  		reftable_record_release(&in);
>
> @@ -143,16 +136,15 @@ static void test_reftable_log_record_equal(void)
>  		}
>  	};
>
> -	EXPECT(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> +	check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
>  	in[1].update_index = in[0].update_index;
> -	EXPECT(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> +	check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
>  	reftable_log_record_release(&in[0]);
>  	reftable_log_record_release(&in[1]);
>  }
>
>  static void test_reftable_log_record_roundtrip(void)
>  {
> -	int i;
>  	struct reftable_log_record in[] = {
>  		{
>  			.refname = xstrdup("refs/heads/master"),
> @@ -180,12 +172,12 @@ static void test_reftable_log_record_roundtrip(void)
>  		}
>  	};
>  	struct strbuf scratch = STRBUF_INIT;
> +	set_hash(in[0].value.update.new_hash, 1);
> +	set_hash(in[0].value.update.old_hash, 2);
> +	set_hash(in[2].value.update.new_hash, 3);
> +	set_hash(in[2].value.update.old_hash, 4);
>
> -	set_test_hash(in[0].value.update.new_hash, 1);
> -	set_test_hash(in[0].value.update.old_hash, 2);
> -	set_test_hash(in[2].value.update.new_hash, 3);
> -	set_test_hash(in[2].value.update.old_hash, 4);
> -	for (i = 0; i < ARRAY_SIZE(in); i++) {
> +	for (size_t i = 0; i < ARRAY_SIZE(in); i++) {
>  		struct reftable_record rec = { .type = BLOCK_TYPE_LOG };
>  		struct strbuf key = STRBUF_INIT;
>  		uint8_t buffer[1024] = { 0 };
> @@ -217,13 +209,13 @@ static void test_reftable_log_record_roundtrip(void)
>  		reftable_record_key(&rec, &key);
>
>  		n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
> -		EXPECT(n >= 0);
> +		check_int(n, >=, 0);
>  		valtype = reftable_record_val_type(&rec);
>  		m = reftable_record_decode(&out, key, valtype, dest,
>  					   GIT_SHA1_RAWSZ, &scratch);
> -		EXPECT(n == m);
> +		check_int(n, ==, m);
>
> -		EXPECT(reftable_log_record_equal(&in[i], &out.u.log,
> +		check(reftable_log_record_equal(&in[i], &out.u.log,
>  						 GIT_SHA1_RAWSZ));
>  		reftable_log_record_release(&in[i]);
>  		strbuf_release(&key);
> @@ -252,14 +244,14 @@ static void test_key_roundtrip(void)
>  	strbuf_addstr(&key, "refs/tags/bla");
>  	extra = 6;
>  	n = reftable_encode_key(&restart, dest, last_key, key, extra);
> -	EXPECT(!restart);
> -	EXPECT(n > 0);
> +	check(!restart);
> +	check_int(n, >, 0);
>
>  	strbuf_addstr(&roundtrip, "refs/heads/master");
>  	m = reftable_decode_key(&roundtrip, &rt_extra, dest);
> -	EXPECT(n == m);
> -	EXPECT(0 == strbuf_cmp(&key, &roundtrip));
> -	EXPECT(rt_extra == extra);
> +	check_int(n, ==, m);
> +	check(!strbuf_cmp(&key, &roundtrip));
> +	check_int(rt_extra, ==, extra);
>
>  	strbuf_release(&last_key);
>  	strbuf_release(&key);
> @@ -289,9 +281,8 @@ static void test_reftable_obj_record_roundtrip(void)
>  		},
>  	};
>  	struct strbuf scratch = STRBUF_INIT;
> -	int i = 0;
>
> -	for (i = 0; i < ARRAY_SIZE(recs); i++) {
> +	for (size_t i = 0; i < ARRAY_SIZE(recs); i++) {
>  		uint8_t buffer[1024] = { 0 };
>  		struct string_view dest = {
>  			.buf = buffer,
> @@ -311,13 +302,13 @@ static void test_reftable_obj_record_roundtrip(void)
>  		test_copy(&in);
>  		reftable_record_key(&in, &key);
>  		n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> -		EXPECT(n > 0);
> +		check_int(n, >, 0);
>  		extra = reftable_record_val_type(&in);
>  		m = reftable_record_decode(&out, key, extra, dest,
>  					   GIT_SHA1_RAWSZ, &scratch);
> -		EXPECT(n == m);
> +		check_int(n, ==, m);
>
> -		EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> +		check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
>  		strbuf_release(&key);
>  		reftable_record_release(&out);
>  	}
> @@ -352,16 +343,16 @@ static void test_reftable_index_record_roundtrip(void)
>  	reftable_record_key(&in, &key);
>  	test_copy(&in);
>
> -	EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key));
> +	check(!strbuf_cmp(&key, &in.u.idx.last_key));
>  	n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> -	EXPECT(n > 0);
> +	check_int(n, >, 0);
>
>  	extra = reftable_record_val_type(&in);
>  	m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ,
>  				   &scratch);
> -	EXPECT(m == n);
> +	check_int(m, ==, n);
>
> -	EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> +	check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
>
>  	reftable_record_release(&out);
>  	strbuf_release(&key);
> @@ -369,14 +360,15 @@ static void test_reftable_index_record_roundtrip(void)
>  	strbuf_release(&in.u.idx.last_key);
>  }
>
> -int record_test_main(int argc, const char *argv[])
> +int cmd_main(int argc, const char *argv[])
>  {
> -	RUN_TEST(test_reftable_log_record_equal);
> -	RUN_TEST(test_reftable_log_record_roundtrip);
> -	RUN_TEST(test_reftable_ref_record_roundtrip);
> -	RUN_TEST(test_varint_roundtrip);
> -	RUN_TEST(test_key_roundtrip);
> -	RUN_TEST(test_reftable_obj_record_roundtrip);
> -	RUN_TEST(test_reftable_index_record_roundtrip);
> -	return 0;
> +	TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
> +	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> +	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> +	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> +	TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work");
> +	TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record");
> +	TEST(test_reftable_index_record_roundtrip(), "record operations work on index record");
> +

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/record_test.c exercises the functions defined in
> > reftable/record.{c, h}. Migrate reftable/record_test.c to the
> > unit testing framework. Migration involves refactoring the tests
> > to use the unit testing framework instead of reftable's test
> > framework.
> > While at it, change the type of index variable 'i' to 'size_t'
> > from 'int'. This is because 'i' is used in comparison against
> > 'ARRAY_SIZE(x)' which is of type 'size_t'.
> >
> > Also, use set_hash() which is defined locally in the test file
> > instead of set_test_hash() which is defined by
> > reftable/test_framework.{c, h}. This is fine to do as both these
> > functions are similarly implemented, and
> > reftable/test_framework.{c, h} is not #included in the ported test.
> >
> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > ---
> >  Makefile                                      |   2 +-
> >  t/helper/test-reftable.c                      |   1 -
> >  .../unit-tests/t-reftable-record.c            | 106 ++++++++----------
> >  3 files changed, 50 insertions(+), 59 deletions(-)
> >  rename reftable/record_test.c => t/unit-tests/t-reftable-record.c (77%)
> >
> > diff --git a/Makefile b/Makefile
> > index f25b2e80a1..def3700b4d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash
> >  UNIT_TEST_PROGRAMS += t-mem-pool
> >  UNIT_TEST_PROGRAMS += t-prio-queue
> >  UNIT_TEST_PROGRAMS += t-reftable-basics
> > +UNIT_TEST_PROGRAMS += t-reftable-record
> >  UNIT_TEST_PROGRAMS += t-strbuf
> >  UNIT_TEST_PROGRAMS += t-strcmp-offset
> >  UNIT_TEST_PROGRAMS += t-strvec
> > @@ -2678,7 +2679,6 @@ REFTABLE_TEST_OBJS += reftable/block_test.o
> >  REFTABLE_TEST_OBJS += reftable/dump.o
> >  REFTABLE_TEST_OBJS += reftable/merged_test.o
> >  REFTABLE_TEST_OBJS += reftable/pq_test.o
> > -REFTABLE_TEST_OBJS += reftable/record_test.o
> >  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
> >  REFTABLE_TEST_OBJS += reftable/stack_test.o
> >  REFTABLE_TEST_OBJS += reftable/test_framework.o
> > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> > index 9160bc5da6..aa6538a8da 100644
> > --- a/t/helper/test-reftable.c
> > +++ b/t/helper/test-reftable.c
> > @@ -5,7 +5,6 @@
> >  int cmd__reftable(int argc, const char **argv)
> >  {
> >       /* test from simple to complex. */
> > -     record_test_main(argc, argv);
> >       block_test_main(argc, argv);
> >       tree_test_main(argc, argv);
> >       pq_test_main(argc, argv);
> > diff --git a/reftable/record_test.c b/t/unit-tests/t-reftable-record.c
> > similarity index 77%
> > rename from reftable/record_test.c
> > rename to t/unit-tests/t-reftable-record.c
> > index 58290bdba3..1b357e6c7f 100644
> > --- a/reftable/record_test.c
> > +++ b/t/unit-tests/t-reftable-record.c
> > @@ -6,13 +6,9 @@
> >    https://developers.google.com/open-source/licenses/bsd
> >  */
> >
> > -#include "record.h"
> > -
> > -#include "system.h"
> > -#include "basics.h"
> > -#include "constants.h"
> > -#include "test_framework.h"
> > -#include "reftable-tests.h"
> > +#include "test-lib.h"
> > +#include "reftable/constants.h"
> > +#include "reftable/record.h"
> >
> >  static void test_copy(struct reftable_record *rec)
> >  {
> > @@ -24,9 +20,9 @@ static void test_copy(struct reftable_record *rec)
> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
> >       /* do it twice to catch memory leaks */
>
> I'm curious why we do this, and if it is still needed. The original
> commit (e303bf22f reftable: (de)serialization for the polymorphic record
> type) doesn't mention any particular reasoning.

> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
> > -     EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> > +     check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> >
> > -     puts("testing print coverage:\n");
> > +     test_msg("testing print coverage:");
> >       reftable_record_print(&copy, GIT_SHA1_RAWSZ);
> >
>
> This prints for any test that uses this function. As I see from the
> current usage of the testing library, we only print debug information
> when we encounter something unexpected.
>
> This also clogs up the unit-test's output. So I would remove this from
> here.

> >       reftable_record_release(&copy);
> > @@ -43,8 +39,8 @@ static void test_varint_roundtrip(void)
> >                             4096,
> >                             ((uint64_t)1 << 63),
> >                             ((uint64_t)1 << 63) + ((uint64_t)1 << 63) - 1 };
> > -     int i = 0;
> > -     for (i = 0; i < ARRAY_SIZE(inputs); i++) {
> > +
> > +     for (size_t i = 0; i < ARRAY_SIZE(inputs); i++) {
> >               uint8_t dest[10];
> >
> >               struct string_view out = {
> > @@ -55,29 +51,26 @@ static void test_varint_roundtrip(void)
> >               int n = put_var_int(&out, in);
> >               uint64_t got = 0;
> >
> > -             EXPECT(n > 0);
> > +             check_int(n, >, 0);
> >               out.len = n;
> >               n = get_var_int(&got, &out);
> > -             EXPECT(n > 0);
> > +             check_int(n, >, 0);
> >
> > -             EXPECT(got == in);
> > +             check_int(got, ==, in);
> >       }
> >  }
> >
> >  static void set_hash(uint8_t *h, int j)
> >  {
> > -     int i = 0;
> > -     for (i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) {
> > +     for (int i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++)
> >               h[i] = (j >> i) & 0xff;
> > -     }
> >  }
> >
> >  static void test_reftable_ref_record_roundtrip(void)
> >  {
> >       struct strbuf scratch = STRBUF_INIT;
> > -     int i = 0;
> >
> > -     for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
> > +     for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
> >               struct reftable_record in = {
> >                       .type = BLOCK_TYPE_REF,
> >               };
> > @@ -109,17 +102,17 @@ static void test_reftable_ref_record_roundtrip(void)
> >
> >               test_copy(&in);
> >
> > -             EXPECT(reftable_record_val_type(&in) == i);
> > +             check_int(reftable_record_val_type(&in), ==, i);
> >
> >               reftable_record_key(&in, &key);
> >               n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> > -             EXPECT(n > 0);
> > +             check_int(n, >, 0);
> >
> >               /* decode into a non-zero reftable_record to test for leaks. */
> >               m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch);
> > -             EXPECT(n == m);
> > +             check_int(n, ==, m);
> >
> > -             EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
> > +             check(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
> >                                                GIT_SHA1_RAWSZ));
> >               reftable_record_release(&in);
> >
> > @@ -143,16 +136,15 @@ static void test_reftable_log_record_equal(void)
> >               }
> >       };
> >
> > -     EXPECT(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> > +     check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> >       in[1].update_index = in[0].update_index;
> > -     EXPECT(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> > +     check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> >       reftable_log_record_release(&in[0]);
> >       reftable_log_record_release(&in[1]);
> >  }
> >
> >  static void test_reftable_log_record_roundtrip(void)
> >  {
> > -     int i;
> >       struct reftable_log_record in[] = {
> >               {
> >                       .refname = xstrdup("refs/heads/master"),
> > @@ -180,12 +172,12 @@ static void test_reftable_log_record_roundtrip(void)
> >               }
> >       };
> >       struct strbuf scratch = STRBUF_INIT;
> > +     set_hash(in[0].value.update.new_hash, 1);
> > +     set_hash(in[0].value.update.old_hash, 2);
> > +     set_hash(in[2].value.update.new_hash, 3);
> > +     set_hash(in[2].value.update.old_hash, 4);
> >
> > -     set_test_hash(in[0].value.update.new_hash, 1);
> > -     set_test_hash(in[0].value.update.old_hash, 2);
> > -     set_test_hash(in[2].value.update.new_hash, 3);
> > -     set_test_hash(in[2].value.update.old_hash, 4);
> > -     for (i = 0; i < ARRAY_SIZE(in); i++) {
> > +     for (size_t i = 0; i < ARRAY_SIZE(in); i++) {
> >               struct reftable_record rec = { .type = BLOCK_TYPE_LOG };
> >               struct strbuf key = STRBUF_INIT;
> >               uint8_t buffer[1024] = { 0 };
> > @@ -217,13 +209,13 @@ static void test_reftable_log_record_roundtrip(void)
> >               reftable_record_key(&rec, &key);
> >
> >               n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
> > -             EXPECT(n >= 0);
> > +             check_int(n, >=, 0);
> >               valtype = reftable_record_val_type(&rec);
> >               m = reftable_record_decode(&out, key, valtype, dest,
> >                                          GIT_SHA1_RAWSZ, &scratch);
> > -             EXPECT(n == m);
> > +             check_int(n, ==, m);
> >
> > -             EXPECT(reftable_log_record_equal(&in[i], &out.u.log,
> > +             check(reftable_log_record_equal(&in[i], &out.u.log,
> >                                                GIT_SHA1_RAWSZ));
> >               reftable_log_record_release(&in[i]);
> >               strbuf_release(&key);
> > @@ -252,14 +244,14 @@ static void test_key_roundtrip(void)
> >       strbuf_addstr(&key, "refs/tags/bla");
> >       extra = 6;
> >       n = reftable_encode_key(&restart, dest, last_key, key, extra);
> > -     EXPECT(!restart);
> > -     EXPECT(n > 0);
> > +     check(!restart);
> > +     check_int(n, >, 0);
> >
> >       strbuf_addstr(&roundtrip, "refs/heads/master");
> >       m = reftable_decode_key(&roundtrip, &rt_extra, dest);
> > -     EXPECT(n == m);
> > -     EXPECT(0 == strbuf_cmp(&key, &roundtrip));
> > -     EXPECT(rt_extra == extra);
> > +     check_int(n, ==, m);
> > +     check(!strbuf_cmp(&key, &roundtrip));
> > +     check_int(rt_extra, ==, extra);
> >
> >       strbuf_release(&last_key);
> >       strbuf_release(&key);
> > @@ -289,9 +281,8 @@ static void test_reftable_obj_record_roundtrip(void)
> >               },
> >       };
> >       struct strbuf scratch = STRBUF_INIT;
> > -     int i = 0;
> >
> > -     for (i = 0; i < ARRAY_SIZE(recs); i++) {
> > +     for (size_t i = 0; i < ARRAY_SIZE(recs); i++) {
> >               uint8_t buffer[1024] = { 0 };
> >               struct string_view dest = {
> >                       .buf = buffer,
> > @@ -311,13 +302,13 @@ static void test_reftable_obj_record_roundtrip(void)
> >               test_copy(&in);
> >               reftable_record_key(&in, &key);
> >               n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> > -             EXPECT(n > 0);
> > +             check_int(n, >, 0);
> >               extra = reftable_record_val_type(&in);
> >               m = reftable_record_decode(&out, key, extra, dest,
> >                                          GIT_SHA1_RAWSZ, &scratch);
> > -             EXPECT(n == m);
> > +             check_int(n, ==, m);
> >
> > -             EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> > +             check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> >               strbuf_release(&key);
> >               reftable_record_release(&out);
> >       }
> > @@ -352,16 +343,16 @@ static void test_reftable_index_record_roundtrip(void)
> >       reftable_record_key(&in, &key);
> >       test_copy(&in);
> >
> > -     EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key));
> > +     check(!strbuf_cmp(&key, &in.u.idx.last_key));
> >       n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
> > -     EXPECT(n > 0);
> > +     check_int(n, >, 0);
> >
> >       extra = reftable_record_val_type(&in);
> >       m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ,
> >                                  &scratch);
> > -     EXPECT(m == n);
> > +     check_int(m, ==, n);
> >
> > -     EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> > +     check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
> >
> >       reftable_record_release(&out);
> >       strbuf_release(&key);
> > @@ -369,14 +360,15 @@ static void test_reftable_index_record_roundtrip(void)
> >       strbuf_release(&in.u.idx.last_key);
> >  }
> >
> > -int record_test_main(int argc, const char *argv[])
> > +int cmd_main(int argc, const char *argv[])
> >  {
> > -     RUN_TEST(test_reftable_log_record_equal);
> > -     RUN_TEST(test_reftable_log_record_roundtrip);
> > -     RUN_TEST(test_reftable_ref_record_roundtrip);
> > -     RUN_TEST(test_varint_roundtrip);
> > -     RUN_TEST(test_key_roundtrip);
> > -     RUN_TEST(test_reftable_obj_record_roundtrip);
> > -     RUN_TEST(test_reftable_index_record_roundtrip);
> > -     return 0;
> > +     TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
> > +     TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> > +     TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> > +     TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> > +     TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work");
> > +     TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record");
> > +     TEST(test_reftable_index_record_roundtrip(), "record operations work on index record");
> > +
>
> All other tests in the 'unit-tests/' folder use a `t_<name>` format for
> the tests. Here we seem to diverge and use a `test_<name>` format. I
> think the best outcome would be some documentation around this, but it
> would still be nice if we follow this pattern nevertheless.
>
> > +     return test_done();
> >  }
> > --
> > 2.45.2.404.g9eaef5822c

> On Tue, 25 Jun 2024 at 13:56, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > reftable/record_test.c exercises the functions defined in
>> > reftable/record.{c, h}. Migrate reftable/record_test.c to the
>> > unit testing framework. Migration involves refactoring the tests
>> > to use the unit testing framework instead of reftable's test
>> > framework.
>> > While at it, change the type of index variable 'i' to 'size_t'
>> > from 'int'. This is because 'i' is used in comparison against
>> > 'ARRAY_SIZE(x)' which is of type 'size_t'.
>> >
>> > Also, use set_hash() which is defined locally in the test file
>> > instead of set_test_hash() which is defined by
>> > reftable/test_framework.{c, h}. This is fine to do as both these
>> > functions are similarly implemented, and
>> > reftable/test_framework.{c, h} is not #included in the ported test.
>> >
>> > Mentored-by: Patrick Steinhardt <ps@pks.im>
>> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>> > ---
>> >  Makefile                                      |   2 +-
>> >  t/helper/test-reftable.c                      |   1 -
>> >  .../unit-tests/t-reftable-record.c            | 106 ++++++++----------
>> >  3 files changed, 50 insertions(+), 59 deletions(-)
>> >  rename reftable/record_test.c => t/unit-tests/t-reftable-record.c (77%)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index f25b2e80a1..def3700b4d 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash
>> >  UNIT_TEST_PROGRAMS += t-mem-pool
>> >  UNIT_TEST_PROGRAMS += t-prio-queue
>> >  UNIT_TEST_PROGRAMS += t-reftable-basics
>> > +UNIT_TEST_PROGRAMS += t-reftable-record
>> >  UNIT_TEST_PROGRAMS += t-strbuf
>> >  UNIT_TEST_PROGRAMS += t-strcmp-offset
>> >  UNIT_TEST_PROGRAMS += t-strvec
>> > @@ -2678,7 +2679,6 @@ REFTABLE_TEST_OBJS += reftable/block_test.o
>> >  REFTABLE_TEST_OBJS += reftable/dump.o
>> >  REFTABLE_TEST_OBJS += reftable/merged_test.o
>> >  REFTABLE_TEST_OBJS += reftable/pq_test.o
>> > -REFTABLE_TEST_OBJS += reftable/record_test.o
>> >  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
>> >  REFTABLE_TEST_OBJS += reftable/stack_test.o
>> >  REFTABLE_TEST_OBJS += reftable/test_framework.o
>> > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
>> > index 9160bc5da6..aa6538a8da 100644
>> > --- a/t/helper/test-reftable.c
>> > +++ b/t/helper/test-reftable.c
>> > @@ -5,7 +5,6 @@
>> >  int cmd__reftable(int argc, const char **argv)
>> >  {
>> >       /* test from simple to complex. */
>> > -     record_test_main(argc, argv);
>> >       block_test_main(argc, argv);
>> >       tree_test_main(argc, argv);
>> >       pq_test_main(argc, argv);
>> > diff --git a/reftable/record_test.c b/t/unit-tests/t-reftable-record.c
>> > similarity index 77%
>> > rename from reftable/record_test.c
>> > rename to t/unit-tests/t-reftable-record.c
>> > index 58290bdba3..1b357e6c7f 100644
>> > --- a/reftable/record_test.c
>> > +++ b/t/unit-tests/t-reftable-record.c
>> > @@ -6,13 +6,9 @@
>> >    https://developers.google.com/open-source/licenses/bsd
>> >  */
>> >
>> > -#include "record.h"
>> > -
>> > -#include "system.h"
>> > -#include "basics.h"
>> > -#include "constants.h"
>> > -#include "test_framework.h"
>> > -#include "reftable-tests.h"
>> > +#include "test-lib.h"
>> > +#include "reftable/constants.h"
>> > +#include "reftable/record.h"
>> >
>> >  static void test_copy(struct reftable_record *rec)
>> >  {
>> > @@ -24,9 +20,9 @@ static void test_copy(struct reftable_record *rec)
>> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>> >       /* do it twice to catch memory leaks */
>>
>> I'm curious why we do this, and if it is still needed. The original
>> commit (e303bf22f reftable: (de)serialization for the polymorphic record
>> type) doesn't mention any particular reasoning.
>
> Yeah, I was confused about this as well. I asked Patrick about it some time
> ago and it seems like he had no clue about it either:
> https://gitlab.slack.com/archives/C071PDKNCHM/p1717479205788209
>

>> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>> > -     EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>> > +     check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>> >
>> > -     puts("testing print coverage:\n");
>> > +     test_msg("testing print coverage:");
>> >       reftable_record_print(&copy, GIT_SHA1_RAWSZ);
>> >
>>
>> This prints for any test that uses this function. As I see from the
>> current usage of the testing library, we only print debug information
>> when we encounter something unexpected.
>>
>> This also clogs up the unit-test's output. So I would remove this from
>> here.
>
> That's true, but that would also mean the print functions are no longer
> exercised. Is that a fine tradeoff?
>

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > On Tue, 25 Jun 2024 at 13:56, Karthik Nayak <karthik.188@gmail.com> wrote:
> >>
> >> Chandra Pratap <chandrapratap3519@gmail.com> writes:
> >>
> >> > reftable/record_test.c exercises the functions defined in
> >> > reftable/record.{c, h}. Migrate reftable/record_test.c to the
> >> > unit testing framework. Migration involves refactoring the tests
> >> > to use the unit testing framework instead of reftable's test
> >> > framework.
> >> > While at it, change the type of index variable 'i' to 'size_t'
> >> > from 'int'. This is because 'i' is used in comparison against
> >> > 'ARRAY_SIZE(x)' which is of type 'size_t'.
> >> >
> >> > Also, use set_hash() which is defined locally in the test file
> >> > instead of set_test_hash() which is defined by
> >> > reftable/test_framework.{c, h}. This is fine to do as both these
> >> > functions are similarly implemented, and
> >> > reftable/test_framework.{c, h} is not #included in the ported test.
> >> >
> >> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> >> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> >> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> >> > ---
> >> >  Makefile                                      |   2 +-
> >> >  t/helper/test-reftable.c                      |   1 -
> >> >  .../unit-tests/t-reftable-record.c            | 106 ++++++++----------
> >> >  3 files changed, 50 insertions(+), 59 deletions(-)
> >> >  rename reftable/record_test.c => t/unit-tests/t-reftable-record.c (77%)
> >> >
> >> > diff --git a/Makefile b/Makefile
> >> > index f25b2e80a1..def3700b4d 100644
> >> > --- a/Makefile
> >> > +++ b/Makefile
> >> > @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash
> >> >  UNIT_TEST_PROGRAMS += t-mem-pool
> >> >  UNIT_TEST_PROGRAMS += t-prio-queue
> >> >  UNIT_TEST_PROGRAMS += t-reftable-basics
> >> > +UNIT_TEST_PROGRAMS += t-reftable-record
> >> >  UNIT_TEST_PROGRAMS += t-strbuf
> >> >  UNIT_TEST_PROGRAMS += t-strcmp-offset
> >> >  UNIT_TEST_PROGRAMS += t-strvec
> >> > @@ -2678,7 +2679,6 @@ REFTABLE_TEST_OBJS += reftable/block_test.o
> >> >  REFTABLE_TEST_OBJS += reftable/dump.o
> >> >  REFTABLE_TEST_OBJS += reftable/merged_test.o
> >> >  REFTABLE_TEST_OBJS += reftable/pq_test.o
> >> > -REFTABLE_TEST_OBJS += reftable/record_test.o
> >> >  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
> >> >  REFTABLE_TEST_OBJS += reftable/stack_test.o
> >> >  REFTABLE_TEST_OBJS += reftable/test_framework.o
> >> > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> >> > index 9160bc5da6..aa6538a8da 100644
> >> > --- a/t/helper/test-reftable.c
> >> > +++ b/t/helper/test-reftable.c
> >> > @@ -5,7 +5,6 @@
> >> >  int cmd__reftable(int argc, const char **argv)
> >> >  {
> >> >       /* test from simple to complex. */
> >> > -     record_test_main(argc, argv);
> >> >       block_test_main(argc, argv);
> >> >       tree_test_main(argc, argv);
> >> >       pq_test_main(argc, argv);
> >> > diff --git a/reftable/record_test.c b/t/unit-tests/t-reftable-record.c
> >> > similarity index 77%
> >> > rename from reftable/record_test.c
> >> > rename to t/unit-tests/t-reftable-record.c
> >> > index 58290bdba3..1b357e6c7f 100644
> >> > --- a/reftable/record_test.c
> >> > +++ b/t/unit-tests/t-reftable-record.c
> >> > @@ -6,13 +6,9 @@
> >> >    https://developers.google.com/open-source/licenses/bsd
> >> >  */
> >> >
> >> > -#include "record.h"
> >> > -
> >> > -#include "system.h"
> >> > -#include "basics.h"
> >> > -#include "constants.h"
> >> > -#include "test_framework.h"
> >> > -#include "reftable-tests.h"
> >> > +#include "test-lib.h"
> >> > +#include "reftable/constants.h"
> >> > +#include "reftable/record.h"
> >> >
> >> >  static void test_copy(struct reftable_record *rec)
> >> >  {
> >> > @@ -24,9 +20,9 @@ static void test_copy(struct reftable_record *rec)
> >> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
> >> >       /* do it twice to catch memory leaks */
> >>
> >> I'm curious why we do this, and if it is still needed. The original
> >> commit (e303bf22f reftable: (de)serialization for the polymorphic record
> >> type) doesn't mention any particular reasoning.
> >
> > Yeah, I was confused about this as well. I asked Patrick about it some time
> > ago and it seems like he had no clue about it either:
> > https://gitlab.slack.com/archives/C071PDKNCHM/p1717479205788209
> >
>
> Just to note, this is an internal GitLab link and not accessible to
> others on the list.
>
> But to summarize, seems like we're not sure why this was added. CC'ing
> Han-Wen here incase he remembers the intent.
>
> > Should we get rid of this after all?
>
> The best solution would be to understand its reasoning and incorporate
> that, but otherwise its best to remove it. We do have CI pipelines to
> capture leaks in a general sense.
>
> >> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
> >> > -     EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> >> > +     check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> >> >
> >> > -     puts("testing print coverage:\n");
> >> > +     test_msg("testing print coverage:");
> >> >       reftable_record_print(&copy, GIT_SHA1_RAWSZ);
> >> >
> >>
> >> This prints for any test that uses this function. As I see from the
> >> current usage of the testing library, we only print debug information
> >> when we encounter something unexpected.
> >>
> >> This also clogs up the unit-test's output. So I would remove this from
> >> here.
> >
> > That's true, but that would also mean the print functions are no longer
> > exercised. Is that a fine tradeoff?
> >
>
> I don't see it this way. Just exercising the function doesn't test it in
> any way. Since the function just prints to stdout without an option to
> pick any other file descriptor, there is no way to test it currently
> either.

> In the current testing setup for log records, only
> reftable_log_record_equal() among log record's comparison functions
> is tested.
>
> Modify the existing tests to exercise reftable_log_record_cmp_void()
> (using the wrapper function reftable_record_cmp()) alongside
> reftable_log_record_equal().
> Note that to achieve this, we'll need to replace instances of
> reftable_log_record_equal() with the wrapper function
> reftable_record_equal().
>
> Rename the now modified test to reflect its nature of exercising
> all comparison operations, not just equality.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 40 +++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 1b357e6c7f..0cf223e51b 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -123,24 +123,38 @@ static void test_reftable_ref_record_roundtrip(void)
>  	strbuf_release(&scratch);
>  }
>
> -static void test_reftable_log_record_equal(void)
> +static void test_reftable_log_record_comparison(void)
>  {
> -	struct reftable_log_record in[2] = {
> +	struct reftable_record in[3] = {
>  		{
> -			.refname = xstrdup("refs/heads/master"),
> -			.update_index = 42,
> +			.type = BLOCK_TYPE_LOG,
> +			.u.log.refname = xstrdup("refs/heads/master"),
> +			.u.log.update_index = 42,
>  		},
>  		{
> -			.refname = xstrdup("refs/heads/master"),
> -			.update_index = 22,
> -		}
> +			.type = BLOCK_TYPE_LOG,
> +			.u.log.refname = xstrdup("refs/heads/master"),
> +			.u.log.update_index = 22,
> +		},
> +		{
> +			.type = BLOCK_TYPE_LOG,
> +			.u.log.refname = xstrdup("refs/heads/main"),
> +			.u.log.update_index = 22,
> +		},
>  	};
>
> -	check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> -	in[1].update_index = in[0].update_index;
> -	check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> -	reftable_log_record_release(&in[0]);
> -	reftable_log_record_release(&in[1]);
> +	check(!reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> +	check(!reftable_record_equal(&in[1], &in[2], GIT_SHA1_RAWSZ));
> +	check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
> +	/* comparison should be reversed for equal keys */

> +	check_int(reftable_record_cmp(&in[0], &in[1]), <, 0);
> +
> +	in[1].u.log.update_index = in[0].u.log.update_index;
> +	check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
> +	check(!reftable_record_cmp(&in[0], &in[1]));
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(in); i++)
> +		reftable_record_release(&in[i]);
>  }
>
>  static void test_reftable_log_record_roundtrip(void)
> @@ -362,7 +376,7 @@ static void test_reftable_index_record_roundtrip(void)
>
>  int cmd_main(int argc, const char *argv[])
>  {
> -	TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
> +	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>  	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>  	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> --
> 2.45.2.404.g9eaef5822c

> reftable_record_is_deletion() is a function defined in
> reftable/record.{c, h} that determines whether a record is of
> type deletion or not. In the current testing setup, this function
> is left untested for all the four record types (ref, log, obj, index).
>
> Add tests for this function in the case of ref records.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 1f9c830631..cbc2ce93b2 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -108,6 +108,7 @@ static void test_reftable_ref_record_roundtrip(void)
>  	for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
>  		struct reftable_record in = {
>  			.type = BLOCK_TYPE_REF,
> +			.u.ref.value_type = i,
>  		};
>  		struct reftable_record out = { .type = BLOCK_TYPE_REF };
>  		struct strbuf key = STRBUF_INIT;
> @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void)
>  		in.u.ref.value_type = i;
>  		switch (i) {
>  		case REFTABLE_REF_DELETION:
> +			check(reftable_record_is_deletion(&in));
>  			break;
>  		case REFTABLE_REF_VAL1:
> +			check(!reftable_record_is_deletion(&in));
>  			set_hash(in.u.ref.value.val1, 1);
>  			break;
>  		case REFTABLE_REF_VAL2:
> +			check(!reftable_record_is_deletion(&in));
>  			set_hash(in.u.ref.value.val2.value, 1);
>  			set_hash(in.u.ref.value.val2.target_value, 2);
>  			break;
>  		case REFTABLE_REF_SYMREF:
> +			check(!reftable_record_is_deletion(&in));
>  			in.u.ref.value.symref = xstrdup("target");
>  			break;
>  		}

> Chandra Pratap <chandrapratap3519@gmail.com> writes:
> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void)
> >               switch (i) {
> >               case REFTABLE_REF_DELETION:
> > +                     check(reftable_record_is_deletion(&in));
> >                       break;
> >               case REFTABLE_REF_VAL1:
> > +                     check(!reftable_record_is_deletion(&in));
> >                       set_hash(in.u.ref.value.val1, 1);
> >                       break;
>
> I think it might be easier to follow if we just move this outside the
> switch, perhaps something like:
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void)
>                 switch (i) {
>                 case REFTABLE_REF_DELETION:
> -                       check(reftable_record_is_deletion(&in));
>                         break;
>                 case REFTABLE_REF_VAL1:
> -                       check(!reftable_record_is_deletion(&in));
>                         set_hash(in.u.ref.value.val1, 1);
>                         break;
> @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void)
> +               check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION);

> On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void)
>> >               switch (i) {
>> >               case REFTABLE_REF_DELETION:
>> > +                     check(reftable_record_is_deletion(&in));
>> >                       break;
>> >               case REFTABLE_REF_VAL1:
>> > +                     check(!reftable_record_is_deletion(&in));
>> >                       set_hash(in.u.ref.value.val1, 1);
>> >                       break;
>>
>> I think it might be easier to follow if we just move this outside the
>> switch, perhaps something like:
>>
>> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void)
>>                 switch (i) {
>>                 case REFTABLE_REF_DELETION:
>> -                       check(reftable_record_is_deletion(&in));
>>                         break;
>>                 case REFTABLE_REF_VAL1:
>> -                       check(!reftable_record_is_deletion(&in));
>>                         set_hash(in.u.ref.value.val1, 1);
>>                         break;
>> @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void)
>> +               check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION);
>
> It's subjective, of course, but for what it's worth, I find Chandra's
> code easier to reason about than this proposed rewrite for at least
> two reasons:
>
> (1) The intention of the original code is obvious at a glance, whereas
> the proposed rewrite requires careful reading and thinking to
> understand what is being tested.
>
> (2) In the original, because the check is being done within each
> `case` arm, it is easy to see how it relates to the case in question,
> whereas in the proposed rewrite, the test is far enough removed from
> from the `switch` that it is more difficult to relate to each possible
> case.

> is left untested for all the four record types (ref, log, obj, index).
>
> Add tests for this function in the case of index records.
> Note that since index records cannot be of type deletion, this function
> must always return '0' when called on an index record.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 766431ca67..99ebfafe0b 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -477,6 +477,7 @@ static void test_reftable_index_record_roundtrip(void)
>  	reftable_record_key(&in, &key);
>  	test_copy(&in);
>
> +	check(!reftable_record_is_deletion(&in));
>  	check(!strbuf_cmp(&key, &in.u.idx.last_key));
>  	n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
>  	check_int(n, >, 0);
> --
> 2.45.2.404.g9eaef5822c

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable_record_is_deletion() is a function defined in
> > reftable/record.{c, h} that determines whether a record is of
> > type deletion or not. In the current testing setup, this function
>
> Nit: 'In the current testing setup' holds true for the series, but on a
> commit level, this statement needs to be modified with each tackled
> type.

> On Tue, 25 Jun 2024 at 14:49, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > reftable_record_is_deletion() is a function defined in
>> > reftable/record.{c, h} that determines whether a record is of
>> > type deletion or not. In the current testing setup, this function
>>
>> Nit: 'In the current testing setup' holds true for the series, but on a
>> commit level, this statement needs to be modified with each tackled
>> type.
>
> I'm not quite sure I follow. Could you explain this a bit further?

> reftable_ref_record_compare_name() is a function defined by
> reftable/record.{c, h} and is used to compare the refname of two
> ref records when sorting multiple ref records using 'qsort'.
> In the current testing setup, this function is left unexercised.
> Add a testing function for the same.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 99ebfafe0b..b949617c88 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -101,6 +101,23 @@ static void test_reftable_ref_record_comparison(void)
>  		reftable_record_release(&in[i]);
>  }
>
> +static void test_reftable_ref_record_compare_name(void)
> +{
> +	struct reftable_ref_record recs[14] = { 0 };
> +	size_t N = ARRAY_SIZE(recs), i;
> +
> +	for (i = 0; i < N; i++)
> +		recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> +
> +	QSORT(recs, N, reftable_ref_record_compare_name);
> +
> +	for (i = 1; i < N; i++)
> +		check(reftable_ref_record_compare_name(&recs[i - 1], &recs[i]) < 0);
> +

> +	for (i = 0; i < N; i++)
> +		reftable_ref_record_release(&recs[i]);
> +}
> +
>  static void test_reftable_ref_record_roundtrip(void)
>  {
>  	struct strbuf scratch = STRBUF_INIT;
> @@ -501,6 +518,7 @@ int cmd_main(int argc, const char *argv[])
>  	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>  	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>  	TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> +	TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
>  	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>  	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> --
> 2.45.2.404.g9eaef5822c

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable_ref_record_compare_name() is a function defined by
> > reftable/record.{c, h} and is used to compare the refname of two
> > ref records when sorting multiple ref records using 'qsort'.
> > In the current testing setup, this function is left unexercised.
> > Add a testing function for the same.
> >
> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > ---
> >  t/unit-tests/t-reftable-record.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> > index 99ebfafe0b..b949617c88 100644
> > --- a/t/unit-tests/t-reftable-record.c
> > +++ b/t/unit-tests/t-reftable-record.c
> > @@ -101,6 +101,23 @@ static void test_reftable_ref_record_comparison(void)
> >               reftable_record_release(&in[i]);
> >  }
> >
> > +static void test_reftable_ref_record_compare_name(void)
> > +{
> > +     struct reftable_ref_record recs[14] = { 0 };
> > +     size_t N = ARRAY_SIZE(recs), i;
> > +
> > +     for (i = 0; i < N; i++)
> > +             recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> > +
> > +     QSORT(recs, N, reftable_ref_record_compare_name);
> > +
> > +     for (i = 1; i < N; i++)
> > +             check(reftable_ref_record_compare_name(&recs[i - 1], &recs[i]) < 0);
> > +
>
> I understand the intention, but using a function to help sort strings
> and then using validating the same function with those strings doesn't
> validate the functionality of the function.

> > +     for (i = 0; i < N; i++)
> > +             reftable_ref_record_release(&recs[i]);
> > +}
> > +
> >  static void test_reftable_ref_record_roundtrip(void)
> >  {
> >       struct strbuf scratch = STRBUF_INIT;
> > @@ -501,6 +518,7 @@ int cmd_main(int argc, const char *argv[])
> >       TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
> >       TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
> >       TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> > +     TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> >       TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> >       TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> >       TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> > --
> > 2.45.2.404.g9eaef5822c

> reftable_log_record_compare_key() is a function defined by
> reftable/record.{c, h} and is used to compare the keys of two
> log records when sorting multiple log records using 'qsort'.
> In the current testing setup, this function is left unexercised.
> Add a testing function for the same.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index b949617c88..d480cc438a 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -214,6 +214,30 @@ static void test_reftable_log_record_comparison(void)
>  		reftable_record_release(&in[i]);
>  }
>
> +static void test_reftable_log_record_compare_key(void)
> +{
> +	struct reftable_log_record logs[14] = { 0 };
> +	size_t N = ARRAY_SIZE(logs), i;
> +
> +	for (i = 0; i < N; i++) {
> +		if (i < N / 2) {
> +			logs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> +			logs[i].update_index = i;
> +		} else {
> +			logs[i].refname = xstrdup("refs/heads/master");
> +			logs[i].update_index = i;
> +		}
> +	}
> +
> +	QSORT(logs, N, reftable_log_record_compare_key);
> +
> +	for (i = 1; i < N; i++)
> +		check(reftable_log_record_compare_key(&logs[i - 1], &logs[i]) < 0);
> +
> +	for (i = 0; i < N; i++)
> +		reftable_log_record_release(&logs[i]);
> +}
> +

>  static void test_reftable_log_record_roundtrip(void)
>  {
>  	struct reftable_log_record in[] = {
> @@ -519,6 +543,7 @@ int cmd_main(int argc, const char *argv[])
>  	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>  	TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
>  	TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> +	TEST(test_reftable_log_record_compare_key(), "reftable_log_record_compare_key works");
>  	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>  	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> --
> 2.45.2.404.g9eaef5822c

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/record_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series moves the test to the unit testing framework,
> and the rest of the patches improve upon the ported test.

> @@ -369,14 +357,15 @@ static void test_reftable_index_record_roundtrip(void)
>  	strbuf_release(&in.u.idx.last_key);
>  }
>
> -int record_test_main(int argc, const char *argv[])
> +int cmd_main(int argc, const char *argv[])
>  {
> -	RUN_TEST(test_reftable_log_record_equal);
> -	RUN_TEST(test_reftable_log_record_roundtrip);
> -	RUN_TEST(test_reftable_ref_record_roundtrip);
> -	RUN_TEST(test_varint_roundtrip);
> -	RUN_TEST(test_key_roundtrip);
> -	RUN_TEST(test_reftable_obj_record_roundtrip);
> -	RUN_TEST(test_reftable_index_record_roundtrip);
> -	return 0;
> +	TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
> +	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> +	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> +	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> +	TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work");
> +	TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record");
> +	TEST(test_reftable_index_record_roundtrip(), "record operations work on index record");
> +
> +	return test_done();
>  }
> --
> 2.45.2.404.g9eaef5822c

> reftable_ref_record_compare_name() is a function defined by
> reftable/record.{c, h} and is used to compare the refname of two
> ref records when sorting multiple ref records using 'qsort'.
> In the current testing setup, this function is left unexercised.
> Add a testing function for the same.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 55b8d03494..f45f2fdef2 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -95,6 +95,28 @@ static void test_reftable_ref_record_comparison(void)
>  	check(!reftable_record_cmp(&in[0], &in[1]));
>  }
>
> +static void test_reftable_ref_record_compare_name(void)
> +{
> +	struct reftable_ref_record recs[14] = { 0 };
> +	size_t N = ARRAY_SIZE(recs), i;
> +
> +	for (i = 0; i < N; i++)
> +		recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);

>  static void test_reftable_ref_record_roundtrip(void)
>  {
>  	struct strbuf scratch = STRBUF_INIT;
> @@ -490,6 +512,7 @@ int cmd_main(int argc, const char *argv[])
>  	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>  	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>  	TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> +	TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
>  	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>  	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> --
> 2.45.2.404.g9eaef5822c

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable_ref_record_compare_name() is a function defined by
> > reftable/record.{c, h} and is used to compare the refname of two
> > ref records when sorting multiple ref records using 'qsort'.
> > In the current testing setup, this function is left unexercised.
> > Add a testing function for the same.
> >
> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > ---
> >  t/unit-tests/t-reftable-record.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> > index 55b8d03494..f45f2fdef2 100644
> > --- a/t/unit-tests/t-reftable-record.c
> > +++ b/t/unit-tests/t-reftable-record.c
> > @@ -95,6 +95,28 @@ static void test_reftable_ref_record_comparison(void)
> >       check(!reftable_record_cmp(&in[0], &in[1]));
> >  }
> >
> > +static void test_reftable_ref_record_compare_name(void)
> > +{
> > +     struct reftable_ref_record recs[14] = { 0 };
> > +     size_t N = ARRAY_SIZE(recs), i;
> > +
> > +     for (i = 0; i < N; i++)
> > +             recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
>
> This needs to be free'd too right?
>
> So we create an array of 14 records, with refnames "00", "01", "02" ...
> "13", here.
>
> > +
> > +     QSORT(recs, N, reftable_ref_record_compare_name);
> > +
>
> We then use `reftable_ref_record_compare_name` as the comparison
> function to sort them.
>
> > +     for (i = 1; i < N; i++) {
> > +             check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0);
> > +             check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0);
> > +     }
>
> Here we use `strcmp` to ensure that the ordering done by
> `reftable_ref_record_compare_name` is correct. This makes sense,
> although I would have expected this to be done the other way around.
> i.e. we should use `strcmp` as the function used in `QSORT` and in this
> loop we validate that `reftable_ref_record_compare_name` also produces
> the same result when comparing.

> > +
> > +     for (i = 0; i < N - 1; i++)
> > +             check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0);
> > +
>
> Also, with the current setup, we use `reftable_ref_record_compare_name`
> to sort the first array and then use `reftable_ref_record_compare_name`
> to check if it is correct? This doesn't work, we need to isolate the
> data creation from the inference, if the same function can influence
> both, then we are not really testing the function.

> > +     for (i = 0; i < N; i++)
> > +             reftable_ref_record_release(&recs[i]);
> > +}
> > +
>
> Nit: The top three loops could possibly be combined.

> >  static void test_reftable_ref_record_roundtrip(void)
> >  {
> >       struct strbuf scratch = STRBUF_INIT;
> > @@ -490,6 +512,7 @@ int cmd_main(int argc, const char *argv[])
> >       TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
> >       TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
> >       TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> > +     TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> >       TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> >       TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> >       TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> > --
> > 2.45.2.404.g9eaef5822c

> On Mon, 1 Jul 2024 at 00:29, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > reftable_ref_record_compare_name() is a function defined by
>> > reftable/record.{c, h} and is used to compare the refname of two
>> > ref records when sorting multiple ref records using 'qsort'.
>> > In the current testing setup, this function is left unexercised.
>> > Add a testing function for the same.
>> >
>> > Mentored-by: Patrick Steinhardt <ps@pks.im>
>> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>> > ---
>> >  t/unit-tests/t-reftable-record.c | 23 +++++++++++++++++++++++
>> >  1 file changed, 23 insertions(+)
>> >
>> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> > index 55b8d03494..f45f2fdef2 100644
>> > --- a/t/unit-tests/t-reftable-record.c
>> > +++ b/t/unit-tests/t-reftable-record.c
>> > @@ -95,6 +95,28 @@ static void test_reftable_ref_record_comparison(void)
>> >       check(!reftable_record_cmp(&in[0], &in[1]));
>> >  }
>> >
>> > +static void test_reftable_ref_record_compare_name(void)
>> > +{
>> > +     struct reftable_ref_record recs[14] = { 0 };
>> > +     size_t N = ARRAY_SIZE(recs), i;
>> > +
>> > +     for (i = 0; i < N; i++)
>> > +             recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
>>
>> This needs to be free'd too right?
>>
>> So we create an array of 14 records, with refnames "00", "01", "02" ...
>> "13", here.
>>
>> > +
>> > +     QSORT(recs, N, reftable_ref_record_compare_name);
>> > +
>>
>> We then use `reftable_ref_record_compare_name` as the comparison
>> function to sort them.
>>
>> > +     for (i = 1; i < N; i++) {
>> > +             check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0);
>> > +             check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0);
>> > +     }
>>
>> Here we use `strcmp` to ensure that the ordering done by
>> `reftable_ref_record_compare_name` is correct. This makes sense,
>> although I would have expected this to be done the other way around.
>> i.e. we should use `strcmp` as the function used in `QSORT` and in this
>> loop we validate that `reftable_ref_record_compare_name` also produces
>> the same result when comparing.
>
> The first parameter to QSORT is an array of 'struct reftable_record' so I don't
> think it's possible to use strcmp() as the comparison function. We do, however,
> use strcmp() internally to compare the ref records.
>

>> > +
>> > +     for (i = 0; i < N - 1; i++)
>> > +             check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0);
>> > +
>>
>> Also, with the current setup, we use `reftable_ref_record_compare_name`
>> to sort the first array and then use `reftable_ref_record_compare_name`
>> to check if it is correct? This doesn't work, we need to isolate the
>> data creation from the inference, if the same function can influence
>> both, then we are not really testing the function.
>
> The validity of `reftable_ref_record_compare_name()` is checked by the first
> loop. Since we're already sure of the order of 'recs' at this point (increasing
> order), this loop is supposed to test the function for ' > 0' case.
>

>> > +     for (i = 0; i < N; i++)
>> > +             reftable_ref_record_release(&recs[i]);
>> > +}
>> > +
>>
>> Nit: The top three loops could possibly be combined.
>
> The limiting as well as initial value for the array indices are all
> different so I'm not sure how to go about this.
>
>> >  static void test_reftable_ref_record_roundtrip(void)
>> >  {
>> >       struct strbuf scratch = STRBUF_INIT;
>> > @@ -490,6 +512,7 @@ int cmd_main(int argc, const char *argv[])
>> >       TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>> >       TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>> >       TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
>> > +     TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
>> >       TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>> >       TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>> >       TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
>> > --
>> > 2.45.2.404.g9eaef5822c

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > On Mon, 1 Jul 2024 at 00:29, Karthik Nayak <karthik.188@gmail.com> wrote:
> >>
> >> Chandra Pratap <chandrapratap3519@gmail.com> writes:
> >>
> >> > reftable_ref_record_compare_name() is a function defined by
> >> > reftable/record.{c, h} and is used to compare the refname of two
> >> > ref records when sorting multiple ref records using 'qsort'.
> >> > In the current testing setup, this function is left unexercised.
> >> > Add a testing function for the same.
> >> >
> >> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> >> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> >> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> >> > ---
> >> >  t/unit-tests/t-reftable-record.c | 23 +++++++++++++++++++++++
> >> >  1 file changed, 23 insertions(+)
> >> >
> >> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> >> > index 55b8d03494..f45f2fdef2 100644
> >> > --- a/t/unit-tests/t-reftable-record.c
> >> > +++ b/t/unit-tests/t-reftable-record.c
> >> > @@ -95,6 +95,28 @@ static void test_reftable_ref_record_comparison(void)
> >> >       check(!reftable_record_cmp(&in[0], &in[1]));
> >> >  }
> >> >
> >> > +static void test_reftable_ref_record_compare_name(void)
> >> > +{
> >> > +     struct reftable_ref_record recs[14] = { 0 };
> >> > +     size_t N = ARRAY_SIZE(recs), i;
> >> > +
> >> > +     for (i = 0; i < N; i++)
> >> > +             recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> >>
> >> This needs to be free'd too right?
> >>
> >> So we create an array of 14 records, with refnames "00", "01", "02" ...
> >> "13", here.
> >>
> >> > +
> >> > +     QSORT(recs, N, reftable_ref_record_compare_name);
> >> > +
> >>
> >> We then use `reftable_ref_record_compare_name` as the comparison
> >> function to sort them.
> >>
> >> > +     for (i = 1; i < N; i++) {
> >> > +             check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0);
> >> > +             check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0);
> >> > +     }
> >>
> >> Here we use `strcmp` to ensure that the ordering done by
> >> `reftable_ref_record_compare_name` is correct. This makes sense,
> >> although I would have expected this to be done the other way around.
> >> i.e. we should use `strcmp` as the function used in `QSORT` and in this
> >> loop we validate that `reftable_ref_record_compare_name` also produces
> >> the same result when comparing.
> >
> > The first parameter to QSORT is an array of 'struct reftable_record' so I don't
> > think it's possible to use strcmp() as the comparison function. We do, however,
> > use strcmp() internally to compare the ref records.
> >
>
> Well, yes, not directly, but you can create your own function and pass
> it to QSORT. This will mostly replicate what
> `reftable_ref_record_compare_name` is doing. But I think you're missing
> what I'm trying to say however.
>
> I'm not really talking about the semantics of it. I'm talking more about
> the concept of it. See the next section...
>
> >> > +
> >> > +     for (i = 0; i < N - 1; i++)
> >> > +             check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0);
> >> > +
> >>
> >> Also, with the current setup, we use `reftable_ref_record_compare_name`
> >> to sort the first array and then use `reftable_ref_record_compare_name`
> >> to check if it is correct? This doesn't work, we need to isolate the
> >> data creation from the inference, if the same function can influence
> >> both, then we are not really testing the function.
> >
> > The validity of `reftable_ref_record_compare_name()` is checked by the first
> > loop. Since we're already sure of the order of 'recs' at this point (increasing
> > order), this loop is supposed to test the function for ' > 0' case.
> >
>
> Yes, the first loop uses 'strcmp' to validate and that's perfectly
> correct. But this operation here is kinda pointless in my opinion. My
> point being that if there is a list x[] and you use a function f() to
> sort that list, validating that x[] is sorted with f() again, doesn't
> test f().
>
> It might be much simpler to just test
> `reftable_ref_record_compare_name()` as so:
>
>     static void test_reftable_ref_record_compare_name(void)
>     {
>         struct reftable_ref_record recs[3] = {
>                 {
>                         .refname = (char *) "refs/heads/a"
>                 },
>                 {
>                         .refname = (char *) "refs/heads/b"
>                 },
>                 {
>                         .refname = (char *) "refs/heads/a"
>                 },
>         };
>
>         check_int(reftable_ref_record_compare_name(&recs[0], &recs[1]), ==, -1);
>         check_int(reftable_ref_record_compare_name(&recs[1], &recs[0]), ==, 1);
>         check_int(reftable_ref_record_compare_name(&recs[0], &recs[2]), ==, 0);
>     }

> >> > +     for (i = 0; i < N; i++)
> >> > +             reftable_ref_record_release(&recs[i]);
> >> > +}
> >> > +
> >>
> >> Nit: The top three loops could possibly be combined.
> >
> > The limiting as well as initial value for the array indices are all
> > different so I'm not sure how to go about this.
> >
> >> >  static void test_reftable_ref_record_roundtrip(void)
> >> >  {
> >> >       struct strbuf scratch = STRBUF_INIT;
> >> > @@ -490,6 +512,7 @@ int cmd_main(int argc, const char *argv[])
> >> >       TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
> >> >       TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
> >> >       TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> >> > +     TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> >> >       TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
> >> >       TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
> >> >       TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> >> > --
> >> > 2.45.2.404.g9eaef5822c

> reftable_log_record_compare_key() is a function defined by
> reftable/record.{c, h} and is used to compare the keys of two
> log records when sorting multiple log records using 'qsort'.
> In the current testing setup, this function is left unexercised.
> Add a testing function for the same.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-record.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index f45f2fdef2..cac8f632f9 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -208,6 +208,37 @@ static void test_reftable_log_record_comparison(void)
>  	check(!reftable_record_cmp(&in[0], &in[1]));
>  }
>
> +static void test_reftable_log_record_compare_key(void)
> +{
> +	struct reftable_log_record logs[14] = { 0 };
> +	size_t N = ARRAY_SIZE(logs), i;
> +
> +	for (i = 0; i < N; i++) {
> +		if (i < N / 2) {
> +			logs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> +			logs[i].update_index = i;
> +		} else {
> +			logs[i].refname = xstrdup("refs/heads/master");
> +			logs[i].update_index = i;
> +		}
> +	}
> +

> +	QSORT(logs, N, reftable_log_record_compare_key);
> +
> +	for (i = 1; i < N / 2; i++)
> +		check_int(strcmp(logs[i - 1].refname, logs[i].refname), <, 0);
> +	for (i = N / 2 + 1; i < N; i++)
> +		check_int(logs[i - 1].update_index, >, logs[i].update_index);
> +
> +	for (i = 0; i < N - 1; i++) {
> +		check_int(reftable_log_record_compare_key(&logs[i], &logs[i]), ==, 0);
> +		check_int(reftable_log_record_compare_key(&logs[i + 1], &logs[i]), >, 0);
> +	}
> +

> +	for (i = 0; i < N; i++)
> +		reftable_log_record_release(&logs[i]);
> +}
> +
>  static void test_reftable_log_record_roundtrip(void)
>  {
>  	struct reftable_log_record in[] = {
> @@ -513,6 +544,7 @@ int cmd_main(int argc, const char *argv[])
>  	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>  	TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
>  	TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> +	TEST(test_reftable_log_record_compare_key(), "reftable_log_record_compare_key works");
>  	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>  	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
> --
> 2.45.2.404.g9eaef5822c

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/record_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series moves the test to the unit testing framework,
> and the rest of the patches improve upon the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v3:
> - Remove reftable_record_print() from tests to reduce clutter in test output.
> - Add more explanation to in-code comment in 1st patch.
> - Use string literals instead of xstrdup() when possible to prevent the cost
>   of mallocing and freeing.
> - make reftable_record_is_deletion() test for refs conciser in 6th patch.
> - Update commit messages for 7th, 8th and 9th patch to better reflect changes.
> - Add tests for '== 0' and '> 0' cases n 10th and 11th patch.
>

> CI/PR: https://github.com/gitgitgadget/git/pull/1750
>
> Chandra Pratap (11):
> t: move reftable/record_test.c to the unit testing framework
> t-reftable-record: add reftable_record_cmp() tests for log records
> t-reftable-record: add comparison tests for ref records
> t-reftable-record: add comparison tests for index records
> t-reftable-record: add comparison tests for obj records
> t-reftable-record: add reftable_record_is_deletion() test for ref records
> t-reftable-record: add reftable_record_is_deletion() test for log records
> t-reftable-record: add reftable_record_is_deletion() test for obj records
> t-reftable-record: add reftable_record_is_deletion() test for index records
> t-reftable-record: add tests for reftable_ref_record_compare_name()
> t-reftable-record: add tests for reftable_log_record_compare_key()
>
> Makefile                         |   2 +-
> reftable/record_test.c           | 382 -------------------------
> t/helper/test-reftable.c         |   1 -
> t/unit-tests/t-reftable-record.c | 554 +++++++++++++++++++++++++++++++++++++++++++++
>
> Range-diff against v2:
>  1:  d0646af549 !  1:  c88fb5bcfa t: move reftable/record_test.c to the unit testing framework
>     @@ Commit message
>          functions are similarly implemented, and
>          reftable/test_framework.{c, h} is not #included in the ported test.
>
>     +    Get rid of reftable_record_print() from the tests as well, because
>     +    it clutters the test framework's output and we have no way of
>     +    verifying the output.
>     +
>          Mentored-by: Patrick Steinhardt <ps@pks.im>
>          Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>     @@ t/unit-tests/t-reftable-record.c: static void test_copy(struct reftable_record *
>       	/* do it twice to catch memory leaks */
>       	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>      -	EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>     -+	check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>     -
>     +-
>      -	puts("testing print coverage:\n");
>     -+	test_msg("testing print coverage:");
>     - 	reftable_record_print(&copy, GIT_SHA1_RAWSZ);
>     +-	reftable_record_print(&copy, GIT_SHA1_RAWSZ);
>     ++	check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>
>       	reftable_record_release(&copy);
>     + }
>      @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void)
>       			      4096,
>       			      ((uint64_t)1 << 63),
>  2:  90feb4168c !  2:  45ac972538 t-reftable-record: add reftable_record_cmp() tests for log records
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>      -			.refname = xstrdup("refs/heads/master"),
>      -			.update_index = 42,
>      +			.type = BLOCK_TYPE_LOG,
>     -+			.u.log.refname = xstrdup("refs/heads/master"),
>     ++			.u.log.refname = (char *) "refs/heads/master",
>      +			.u.log.update_index = 42,
>       		},
>       		{
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>      -			.update_index = 22,
>      -		}
>      +			.type = BLOCK_TYPE_LOG,
>     -+			.u.log.refname = xstrdup("refs/heads/master"),
>     ++			.u.log.refname = (char *) "refs/heads/master",
>      +			.u.log.update_index = 22,
>      +		},
>      +		{
>      +			.type = BLOCK_TYPE_LOG,
>     -+			.u.log.refname = xstrdup("refs/heads/main"),
>     ++			.u.log.refname = (char *) "refs/heads/main",
>      +			.u.log.update_index = 22,
>      +		},
>       	};
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>      +	check(!reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
>      +	check(!reftable_record_equal(&in[1], &in[2], GIT_SHA1_RAWSZ));
>      +	check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
>     -+	/* comparison should be reversed for equal keys */
>     ++	/* comparison should be reversed for equal keys, because
>     ++	 * comparison is now performed on the basis of update indices */
>      +	check_int(reftable_record_cmp(&in[0], &in[1]), <, 0);
>      +
>      +	in[1].u.log.update_index = in[0].u.log.update_index;
>      +	check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
>      +	check(!reftable_record_cmp(&in[0], &in[1]));
>     -+
>     -+	for (size_t i = 0; i < ARRAY_SIZE(in); i++)
>     -+		reftable_record_release(&in[i]);
>       }
>
>       static void test_reftable_log_record_roundtrip(void)
>  3:  e435166a78 !  3:  db76851f4b t-reftable-record: add comparison tests for ref records
>     @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j)
>      +	struct reftable_record in[3] = {
>      +		{
>      +			.type = BLOCK_TYPE_REF,
>     -+			.u.ref.refname = xstrdup("refs/heads/master"),
>     ++			.u.ref.refname = (char *) "refs/heads/master",
>      +			.u.ref.value_type = REFTABLE_REF_VAL1,
>      +		},
>      +		{
>      +			.type = BLOCK_TYPE_REF,
>     -+			.u.ref.refname = xstrdup("refs/heads/master"),
>     ++			.u.ref.refname = (char *) "refs/heads/master",
>      +			.u.ref.value_type = REFTABLE_REF_DELETION,
>      +		},
>      +		{
>      +			.type = BLOCK_TYPE_REF,
>     -+			.u.ref.refname = xstrdup("HEAD"),
>     ++			.u.ref.refname = (char *) "HEAD",
>      +			.u.ref.value_type = REFTABLE_REF_SYMREF,
>     -+			.u.ref.value.symref = xstrdup("refs/heads/master"),
>     ++			.u.ref.value.symref = (char *) "refs/heads/master",
>      +		},
>      +	};
>      +
>     @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j)
>      +	in[1].u.ref.value_type = in[0].u.ref.value_type;
>      +	check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ));
>      +	check(!reftable_record_cmp(&in[0], &in[1]));
>     -+
>     -+	for (size_t i = 0; i < ARRAY_SIZE(in); i++)
>     -+		reftable_record_release(&in[i]);
>      +}
>      +
>       static void test_reftable_ref_record_roundtrip(void)
>  4:  ad014db045 =  4:  78aff923c6 t-reftable-record: add comparison tests for index records
>  5:  69c1f3891a =  5:  b0b3c98042 t-reftable-record: add comparison tests for obj records
>  6:  dca1a016da <  -:  ---------- t-reftable-record: add ref tests for reftable_record_is_deletion()
>  -:  ---------- >  6:  5e6b004216 t-reftable-record: add ref tests for reftable_record_is_deletion()
>  7:  c7ffff71b0 !  7:  a68be88ccb t-reftable-record: add log tests for reftable_record_is_deletion()
>     @@ Commit message
>          reftable_record_is_deletion() is a function defined in
>          reftable/record.{c, h} that determines whether a record is of
>          type deletion or not. In the current testing setup, this function
>     -    is left untested for all the four record types (ref, log, obj, index).
>     +    is left untested for three of the four record types (log, obj, index).
>
>          Add tests for this function in the case of log records.
>
>  8:  f3e0c2aaf5 !  8:  02516add15 t-reftable-record: add obj tests for reftable_record_is_deletion()
>     @@ Commit message
>          reftable_record_is_deletion() is a function defined in
>          reftable/record.{c, h} that determines whether a record is of
>          type deletion or not. In the current testing setup, this function
>     -    is left untested for all the four record types (ref, log, obj, index).
>     +    is left untested for two of the four record types (obj, index).
>
>          Add tests for this function in the case of obj records.
>          Note that since obj records cannot be of type deletion, this function
>  9:  8eeeb63982 !  9:  541f9811d3 t-reftable-record: add index tests for reftable_record_is_deletion()
>     @@ Commit message
>          reftable_record_is_deletion() is a function defined in
>          reftable/record.{c, h} that determines whether a record is of
>          type deletion or not. In the current testing setup, this function
>     -    is left untested for all the four record types (ref, log, obj, index).
>     +    is left untested for index records.
>
>          Add tests for this function in the case of index records.
>          Note that since index records cannot be of type deletion, this function
> 10:  979db146a0 ! 10:  c2aff283b1 t-reftable-record: add tests for reftable_ref_record_compare_name()
>     @@ Commit message
>
>       ## t/unit-tests/t-reftable-record.c ##
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_comparison(void)
>     - 		reftable_record_release(&in[i]);
>     + 	check(!reftable_record_cmp(&in[0], &in[1]));
>       }
>
>      +static void test_reftable_ref_record_compare_name(void)
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_compariso
>      +
>      +	QSORT(recs, N, reftable_ref_record_compare_name);
>      +
>     -+	for (i = 1; i < N; i++)
>     -+		check(reftable_ref_record_compare_name(&recs[i - 1], &recs[i]) < 0);
>     ++	for (i = 1; i < N; i++) {
>     ++		check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0);
>     ++		check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0);
>     ++	}
>     ++
>     ++	for (i = 0; i < N - 1; i++)
>     ++		check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0);
>      +
>      +	for (i = 0; i < N; i++)
>      +		reftable_ref_record_release(&recs[i]);
> 11:  fe044f186b ! 11:  7bdfca3744 t-reftable-record: add tests for reftable_log_record_compare_key()
>     @@ Commit message
>
>       ## t/unit-tests/t-reftable-record.c ##
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_comparison(void)
>     - 		reftable_record_release(&in[i]);
>     + 	check(!reftable_record_cmp(&in[0], &in[1]));
>       }
>
>      +static void test_reftable_log_record_compare_key(void)
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_compariso
>      +
>      +	QSORT(logs, N, reftable_log_record_compare_key);
>      +
>     -+	for (i = 1; i < N; i++)
>     -+		check(reftable_log_record_compare_key(&logs[i - 1], &logs[i]) < 0);
>     ++	for (i = 1; i < N / 2; i++)
>     ++		check_int(strcmp(logs[i - 1].refname, logs[i].refname), <, 0);
>     ++	for (i = N / 2 + 1; i < N; i++)
>     ++		check_int(logs[i - 1].update_index, >, logs[i].update_index);
>     ++
>     ++	for (i = 0; i < N - 1; i++) {
>     ++		check_int(reftable_log_record_compare_key(&logs[i], &logs[i]), ==, 0);
>     ++		check_int(reftable_log_record_compare_key(&logs[i + 1], &logs[i]), >, 0);
>     ++	}
>      +
>      +	for (i = 0; i < N; i++)
>      +		reftable_log_record_release(&logs[i]);

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/record_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series moves the test to the unit testing framework,
> and the rest of the patches improve upon the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v4:
> - Rename the tests to follow a 't_foo()' pattern instead
>   of 'test_foo()'
> - Use hard-coded test input in the 10th and 11th patch
>

> CI/PR: https://github.com/gitgitgadget/git/pull/1750
>
> Chandra Pratap (11):
> t: move reftable/record_test.c to the unit testing framework
> t-reftable-record: add reftable_record_cmp() tests for log records
> t-reftable-record: add comparison tests for ref records
> t-reftable-record: add comparison tests for index records
> t-reftable-record: add comparison tests for obj records
> t-reftable-record: add reftable_record_is_deletion() test for ref records
> t-reftable-record: add reftable_record_is_deletion() test for log records
> t-reftable-record: add reftable_record_is_deletion() test for obj records
> t-reftable-record: add reftable_record_is_deletion() test for index records
> t-reftable-record: add tests for reftable_ref_record_compare_name()
> t-reftable-record: add tests for reftable_log_record_compare_key()
>
> Makefile                         |   2 +-
> reftable/record_test.c           | 382 -------------------------
> t/helper/test-reftable.c         |   1 -
> t/unit-tests/t-reftable-record.c | 551 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 552 insertions(+), 384 deletions(-)
>
> Range-diff against v3:
>  1:  c88fb5bcfa !  1:  c210a0da8f t: move reftable/record_test.c to the unit testing framework
>     @@ Commit message
>          reftable/record.{c, h}. Migrate reftable/record_test.c to the
>          unit testing framework. Migration involves refactoring the tests
>          to use the unit testing framework instead of reftable's test
>     -    framework.
>     +    framework, and renaming the tests to fit unit-tests' naming scheme.
>     +
>          While at it, change the type of index variable 'i' to 'size_t'
>          from 'int'. This is because 'i' is used in comparison against
>          'ARRAY_SIZE(x)' which is of type 'size_t'.
>     @@ reftable/record_test.c => t/unit-tests/t-reftable-record.c
>       */
>
>      -#include "record.h"
>     --
>     ++#include "test-lib.h"
>     ++#include "reftable/constants.h"
>     ++#include "reftable/record.h"
>     +
>      -#include "system.h"
>      -#include "basics.h"
>      -#include "constants.h"
>      -#include "test_framework.h"
>      -#include "reftable-tests.h"
>     -+#include "test-lib.h"
>     -+#include "reftable/constants.h"
>     -+#include "reftable/record.h"
>     -
>     - static void test_copy(struct reftable_record *rec)
>     +-
>     +-static void test_copy(struct reftable_record *rec)
>     ++static void t_copy(struct reftable_record *rec)
>       {
>     + 	struct reftable_record copy;
>     + 	uint8_t typ;
>      @@ t/unit-tests/t-reftable-record.c: static void test_copy(struct reftable_record *rec)
>       	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>       	/* do it twice to catch memory leaks */
>     @@ t/unit-tests/t-reftable-record.c: static void test_copy(struct reftable_record *
>
>       	reftable_record_release(&copy);
>       }
>     +
>     +-static void test_varint_roundtrip(void)
>     ++static void t_varint_roundtrip(void)
>     + {
>     + 	uint64_t inputs[] = { 0,
>     + 			      1,
>      @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void)
>       			      4096,
>       			      ((uint64_t)1 << 63),
>     @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void)
>      -	}
>       }
>
>     - static void test_reftable_ref_record_roundtrip(void)
>     +-static void test_reftable_ref_record_roundtrip(void)
>     ++static void t_reftable_ref_record_roundtrip(void)
>       {
>       	struct strbuf scratch = STRBUF_INIT;
>      -	int i = 0;
>     @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void)
>       			.type = BLOCK_TYPE_REF,
>       		};
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip(void)
>     + 		}
>     + 		in.u.ref.refname = xstrdup("refs/heads/master");
>
>     - 		test_copy(&in);
>     +-		test_copy(&in);
>     ++		t_copy(&in);
>
>      -		EXPECT(reftable_record_val_type(&in) == i);
>      +		check_int(reftable_record_val_type(&in), ==, i);
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>       						 GIT_SHA1_RAWSZ));
>       		reftable_record_release(&in);
>
>     +@@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip(void)
>     + 	strbuf_release(&scratch);
>     + }
>     +
>     +-static void test_reftable_log_record_equal(void)
>     ++static void t_reftable_log_record_equal(void)
>     + {
>     + 	struct reftable_log_record in[2] = {
>     + 		{
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_equal(void)
>       		}
>       	};
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_equal(voi
>       	reftable_log_record_release(&in[1]);
>       }
>
>     - static void test_reftable_log_record_roundtrip(void)
>     +-static void test_reftable_log_record_roundtrip(void)
>     ++static void t_reftable_log_record_roundtrip(void)
>       {
>      -	int i;
>       	struct reftable_log_record in[] = {
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_roundtrip
>       		struct strbuf key = STRBUF_INIT;
>       		uint8_t buffer[1024] = { 0 };
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_roundtrip(void)
>     +
>     + 		rec.u.log = in[i];
>     +
>     +-		test_copy(&rec);
>     ++		t_copy(&rec);
>     +
>       		reftable_record_key(&rec, &key);
>
>       		n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_roundtrip
>       						 GIT_SHA1_RAWSZ));
>       		reftable_log_record_release(&in[i]);
>       		strbuf_release(&key);
>     +@@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_roundtrip(void)
>     + 	strbuf_release(&scratch);
>     + }
>     +
>     +-static void test_key_roundtrip(void)
>     ++static void t_key_roundtrip(void)
>     + {
>     + 	uint8_t buffer[1024] = { 0 };
>     + 	struct string_view dest = {
>      @@ t/unit-tests/t-reftable-record.c: static void test_key_roundtrip(void)
>       	strbuf_addstr(&key, "refs/tags/bla");
>       	extra = 6;
>     @@ t/unit-tests/t-reftable-record.c: static void test_key_roundtrip(void)
>
>       	strbuf_release(&last_key);
>       	strbuf_release(&key);
>     + 	strbuf_release(&roundtrip);
>     + }
>     +
>     +-static void test_reftable_obj_record_roundtrip(void)
>     ++static void t_reftable_obj_record_roundtrip(void)
>     + {
>     + 	uint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };
>     + 	uint64_t till9[] = { 1, 2, 3, 4, 500, 600, 700, 800, 9000 };
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip(void)
>       		},
>       	};
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip
>       		struct string_view dest = {
>       			.buf = buffer,
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip(void)
>     - 		test_copy(&in);
>     + 		int n, m;
>     + 		uint8_t extra;
>     +
>     +-		test_copy(&in);
>     ++		t_copy(&in);
>       		reftable_record_key(&in, &key);
>       		n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
>      -		EXPECT(n > 0);
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip
>       		strbuf_release(&key);
>       		reftable_record_release(&out);
>       	}
>     +@@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip(void)
>     + 	strbuf_release(&scratch);
>     + }
>     +
>     +-static void test_reftable_index_record_roundtrip(void)
>     ++static void t_reftable_index_record_roundtrip(void)
>     + {
>     + 	struct reftable_record in = {
>     + 		.type = BLOCK_TYPE_INDEX,
>      @@ t/unit-tests/t-reftable-record.c: static void test_reftable_index_record_roundtrip(void)
>     +
>     + 	strbuf_addstr(&in.u.idx.last_key, "refs/heads/master");
>       	reftable_record_key(&in, &key);
>     - 	test_copy(&in);
>     +-	test_copy(&in);
>     ++	t_copy(&in);
>
>      -	EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key));
>      +	check(!strbuf_cmp(&key, &in.u.idx.last_key));
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_index_record_roundtr
>      -	RUN_TEST(test_reftable_obj_record_roundtrip);
>      -	RUN_TEST(test_reftable_index_record_roundtrip);
>      -	return 0;
>     -+	TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
>     -+	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>     -+	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     -+	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
>     -+	TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work");
>     -+	TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record");
>     -+	TEST(test_reftable_index_record_roundtrip(), "record operations work on index record");
>     ++	TEST(t_reftable_log_record_equal(), "reftable_log_record_equal works");
>     ++	TEST(t_reftable_log_record_roundtrip(), "record operations work on log record");
>     ++	TEST(t_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     ++	TEST(t_varint_roundtrip(), "put_var_int and get_var_int work");
>     ++	TEST(t_key_roundtrip(), "reftable_encode_key and reftable_decode_key work");
>     ++	TEST(t_reftable_obj_record_roundtrip(), "record operations work on obj record");
>     ++	TEST(t_reftable_index_record_roundtrip(), "record operations work on index record");
>      +
>      +	return test_done();
>       }
>  2:  45ac972538 !  2:  1e8a229cbd t-reftable-record: add reftable_record_cmp() tests for log records
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_ref_record_roundtrip(void)
>       	strbuf_release(&scratch);
>       }
>
>     --static void test_reftable_log_record_equal(void)
>     -+static void test_reftable_log_record_comparison(void)
>     +-static void t_reftable_log_record_equal(void)
>     ++static void t_reftable_log_record_comparison(void)
>       {
>      -	struct reftable_log_record in[2] = {
>      +	struct reftable_record in[3] = {
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>      +	check(!reftable_record_cmp(&in[0], &in[1]));
>       }
>
>     - static void test_reftable_log_record_roundtrip(void)
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_index_record_roundtrip(void)
>     + static void t_reftable_log_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_index_record_roundtrip(void)
>
>       int cmd_main(int argc, const char *argv[])
>       {
>     --	TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works");
>     -+	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>     - 	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>     - 	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     - 	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
>     +-	TEST(t_reftable_log_record_equal(), "reftable_log_record_equal works");
>     ++	TEST(t_reftable_log_record_comparison(), "comparison operations work on log record");
>     + 	TEST(t_reftable_log_record_roundtrip(), "record operations work on log record");
>     + 	TEST(t_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     + 	TEST(t_varint_roundtrip(), "put_var_int and get_var_int work");
>  3:  db76851f4b !  3:  f45611e493 t-reftable-record: add comparison tests for ref records
>     @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j)
>       		h[i] = (j >> i) & 0xff;
>       }
>
>     -+static void test_reftable_ref_record_comparison(void)
>     ++static void t_reftable_ref_record_comparison(void)
>      +{
>      +	struct reftable_record in[3] = {
>      +		{
>     @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j)
>      +	check(!reftable_record_cmp(&in[0], &in[1]));
>      +}
>      +
>     - static void test_reftable_ref_record_roundtrip(void)
>     + static void t_reftable_ref_record_roundtrip(void)
>       {
>       	struct strbuf scratch = STRBUF_INIT;
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_index_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_index_record_roundtrip(void)
>
>       int cmd_main(int argc, const char *argv[])
>       {
>     -+	TEST(test_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     - 	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>     - 	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>     - 	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     ++	TEST(t_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     + 	TEST(t_reftable_log_record_comparison(), "comparison operations work on log record");
>     + 	TEST(t_reftable_log_record_roundtrip(), "record operations work on log record");
>     + 	TEST(t_reftable_ref_record_roundtrip(), "record operations work on ref record");
>  4:  78aff923c6 !  4:  28387b65e0 t-reftable-record: add comparison tests for index records
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_obj_record_roundtrip(void)
>       	strbuf_release(&scratch);
>       }
>
>     -+static void test_reftable_index_record_comparison(void)
>     ++static void t_reftable_index_record_comparison(void)
>      +{
>      +	struct reftable_record in[3] = {
>      +		{
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_obj_record_roundtrip
>      +		reftable_record_release(&in[i]);
>      +}
>      +
>     - static void test_reftable_index_record_roundtrip(void)
>     + static void t_reftable_index_record_roundtrip(void)
>       {
>       	struct reftable_record in = {
>      @@ t/unit-tests/t-reftable-record.c: int cmd_main(int argc, const char *argv[])
>       {
>     - 	TEST(test_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     - 	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>     -+	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>     - 	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>     - 	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     - 	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
>     + 	TEST(t_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     + 	TEST(t_reftable_log_record_comparison(), "comparison operations work on log record");
>     ++	TEST(t_reftable_index_record_comparison(), "comparison operations work on index record");
>     + 	TEST(t_reftable_log_record_roundtrip(), "record operations work on log record");
>     + 	TEST(t_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     + 	TEST(t_varint_roundtrip(), "put_var_int and get_var_int work");
>  5:  b0b3c98042 !  5:  6349ce15f4 t-reftable-record: add comparison tests for obj records
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_key_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_key_roundtrip(void)
>       	strbuf_release(&roundtrip);
>       }
>
>     -+static void test_reftable_obj_record_comparison(void)
>     ++static void t_reftable_obj_record_comparison(void)
>      +{
>      +
>      +	uint8_t id_bytes[] = { 0, 1, 2, 3, 4, 5, 6 };
>     @@ t/unit-tests/t-reftable-record.c: static void test_key_roundtrip(void)
>      +	check(!reftable_record_cmp(&in[0], &in[1]));
>      +}
>      +
>     - static void test_reftable_obj_record_roundtrip(void)
>     + static void t_reftable_obj_record_roundtrip(void)
>       {
>       	uint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };
>      @@ t/unit-tests/t-reftable-record.c: int cmd_main(int argc, const char *argv[])
>     - 	TEST(test_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     - 	TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>     - 	TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
>     -+	TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
>     - 	TEST(test_reftable_log_record_roundtrip(), "record operations work on log record");
>     - 	TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     - 	TEST(test_varint_roundtrip(), "put_var_int and get_var_int work");
>     + 	TEST(t_reftable_ref_record_comparison(), "comparison operations work on ref record");
>     + 	TEST(t_reftable_log_record_comparison(), "comparison operations work on log record");
>     + 	TEST(t_reftable_index_record_comparison(), "comparison operations work on index record");
>     ++	TEST(t_reftable_obj_record_comparison(), "comparison operations work on obj record");
>     + 	TEST(t_reftable_log_record_roundtrip(), "record operations work on log record");
>     + 	TEST(t_reftable_ref_record_roundtrip(), "record operations work on ref record");
>     + 	TEST(t_varint_roundtrip(), "put_var_int and get_var_int work");
>  6:  5e6b004216 !  6:  9202c783b9 t-reftable-record: add ref tests for reftable_record_is_deletion()
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_ref_record_roundtrip(void)
>       	for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
>       		struct reftable_record in = {
>       			.type = BLOCK_TYPE_REF,
>     @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip
>       		};
>       		struct reftable_record out = { .type = BLOCK_TYPE_REF };
>       		struct strbuf key = STRBUF_INIT;
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip(void)
>     - 		test_copy(&in);
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_ref_record_roundtrip(void)
>     + 		t_copy(&in);
>
>       		check_int(reftable_record_val_type(&in), ==, i);
>      +		check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION);
>  7:  a68be88ccb !  7:  4632a00e15 t-reftable-record: add log tests for reftable_record_is_deletion()
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_log_record_roundtrip(void)
>       	set_hash(in[2].value.update.new_hash, 3);
>       	set_hash(in[2].value.update.old_hash, 4);
>
>  8:  02516add15 <  -:  ---------- t-reftable-record: add obj tests for reftable_record_is_deletion()
>  -:  ---------- >  8:  3826ed5ef3 t-reftable-record: add obj tests for reftable_record_is_deletion()
>  9:  541f9811d3 !  9:  1fba6d500c t-reftable-record: add index tests for reftable_record_is_deletion()
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-record.c ##
>     -@@ t/unit-tests/t-reftable-record.c: static void test_reftable_index_record_roundtrip(void)
>     +@@ t/unit-tests/t-reftable-record.c: static void t_reftable_index_record_roundtrip(void)
>       	reftable_record_key(&in, &key);
>     - 	test_copy(&in);
>     + 	t_copy(&in);
>
>      +	check(!reftable_record_is_deletion(&in));
>       	check(!strbuf_cmp(&key, &in.u.idx.last_key));
> 10:  c2aff283b1 <  -:  ---------- t-reftable-record: add tests for reftable_ref_record_compare_name()
> 11:  7bdfca3744 <  -:  ---------- t-reftable-record: add tests for reftable_log_record_compare_key()
>  -:  ---------- > 10:  3511e36c18 t-reftable-record: add tests for reftable_ref_record_compare_name()
>  -:  ---------- > 11:  97e5cbaeaf t-reftable-record: add tests for reftable_log_record_compare_key()

> Hello,
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
>> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
>> testing framework written entirely in C was introduced to the Git project
>> aimed at simplifying testing and reducing test run times.
>> Currently, tests for the reftable refs-backend are performed by a custom
>> testing framework defined by reftable/test_framework.{c, h}. Port
>> reftable/record_test.c to the unit testing framework and improve upon
>> the ported test.
>>
>> The first patch in the series moves the test to the unit testing framework,
>> and the rest of the patches improve upon the ported test.
>>
>> Mentored-by: Patrick Steinhardt <ps@pks.im>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>>
>> ---
>> Changes in v4:
>> - Rename the tests to follow a 't_foo()' pattern instead
>>   of 'test_foo()'
>> - Use hard-coded test input in the 10th and 11th patch
>>
>
> Nice, I see my comments from the previous versions are addressed. It
> looks good to me now!