[GSoC][PATCH 0/5] t: port reftable/readwrite_test.c to the unit testing framework

[GSoC][PATCH 0/5] t: port reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/readwrite_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>

---
CI/PR: https://github.com/gitgitgadget/git/pull/1770

Chandra Pratap(5):
t: move reftable/readwrite_test.c to the unit testing framework
t-reftable-readwrite: use free_names() instead of a for loop
t-reftable-readwrite: use 'for' in place of infinite 'while' loops
t-reftable-readwrite: add test for known error
t-reftable-readwrite: add tests for print functions

Makefile                                                         |   2 +-
reftable/reftable-tests.h                                        |   1 -
t/helper/test-reftable.c                                         |   1 -
reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 517 +++++++++++++++++++++++-----------------
4 files changed, 294 insertions(+), 227 deletions(-)

[PATCH 1/5] t: move reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

reftable/readwrite_test.c exercises the functions defined in
reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
reftable/readwrite_test.c to the unit testing framework. Migration
involves refactoring the tests to use the unit testing framework
instead of reftable's test framework and renaming the tests to
align with unit-tests' naming conventions.

Since some tests in reftable/readwrite_test.c use the functions
set_test_hash(), noop_flush() and strbuf_add_void() defined in
reftable/test_framework.{c,h} but these files are not #included
in the ported unit test, copy these functions in the new test file.

While at it, ensure structs are 0-initialized with '= { 0 }'
instead of '= { NULL }'.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 reftable/reftable-tests.h                     |   1 -
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------
 4 files changed, 210 insertions(+), 212 deletions(-)
 rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)

diff --git a/Makefile b/Makefile
index 3863e60b66..76e4d1c1ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-merged
+UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..7d955393d2 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);
 int block_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
-int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9d378427da..d371e9f9dd 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	block_test_main(argc, argv);
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
-	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c
similarity index 70%
rename from reftable/readwrite_test.c
rename to t/unit-tests/t-reftable-readwrite.c
index f411abfe9c..235e3d94c7 100644
--- a/reftable/readwrite_test.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-
-#include "basics.h"
-#include "block.h"
-#include "blocksource.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-#include "reftable-writer.h"
+#include "test-lib.h"
+#include "reftable/reader.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-writer.h"
 
 static const int update_index = 5;
 
-static void test_buffer(void)
+static void set_test_hash(uint8_t *p, int i)
+{
+	memset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));
+}
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
+
+static int noop_flush(void *arg)
+{
+	return 0;
+}
+
+static void t_buffer(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_block out = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_block out = { 0 };
 	int n;
 	uint8_t in[] = "hello";
 	strbuf_add(&buf, in, sizeof(in));
 	block_source_from_strbuf(&source, &buf);
-	EXPECT(block_source_size(&source) == 6);
+	check_int(block_source_size(&source), ==, 6);
 	n = block_source_read_block(&source, &out, 0, sizeof(in));
-	EXPECT(n == sizeof(in));
-	EXPECT(!memcmp(in, out.data, n));
+	check_int(n, ==, sizeof(in));
+	check(!memcmp(in, out.data, n));
 	reftable_block_done(&out);
 
 	n = block_source_read_block(&source, &out, 1, 2);
-	EXPECT(n == 2);
-	EXPECT(!memcmp(out.data, "el", 2));
+	check_int(n, ==, 2);
+	check(!memcmp(out.data, "el", 2));
 
 	reftable_block_done(&out);
 	block_source_close(&source);
@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 	};
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0, n;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	const struct reftable_stats *stats = NULL;
 
 	REFTABLE_CALLOC_ARRAY(*names, N + 1);
@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		(*names)[i] = xstrdup(name);
 
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	for (i = 0; i < N; i++) {
@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		log.value.update.message = (char *) "message";
 
 		n = reftable_writer_add_log(w, &log);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
 	for (i = 0; i < stats->ref_stats.blocks; i++) {
 		int off = i * opts.block_size;
-		if (off == 0) {
-			off = header_size(
-				(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
-		}
-		EXPECT(buf->buf[off] == 'r');
+		if (!off)
+			off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
+		check(buf->buf[off] == 'r');
 	}
 
-	EXPECT(stats->log_stats.blocks > 0);
+	check(stats->log_stats.blocks > 0);
 	reftable_writer_free(w);
 }
 
-static void test_log_buffer_size(void)
+static void t_log_buffer_size(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_write_options opts = {
@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)
 	}
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_overflow(void)
+static void t_log_overflow(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	char msg[256] = { 0 };
@@ -177,12 +186,12 @@ static void test_log_overflow(void)
 	memset(msg, 'x', sizeof(msg) - 1);
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);
+	check_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_write_read(void)
+static void t_log_write_read(void)
 {
 	int N = 2;
 	char **names = reftable_calloc(N + 1, sizeof(*names));
@@ -190,13 +199,13 @@ static void test_log_write_read(void)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	int n;
-	struct reftable_iterator it = { NULL };
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
@@ -204,17 +213,17 @@ static void test_log_write_read(void)
 	reftable_writer_set_limits(w, 0, N);
 	for (i = 0; i < N; i++) {
 		char name[256];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		snprintf(name, sizeof(name), "b%02d%0*d", i, 130, 7);
 		names[i] = xstrdup(name);
 		ref.refname = name;
 		ref.update_index = i;
 
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	for (i = 0; i < N; i++) {
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 
 		log.refname = names[i];
 		log.update_index = i;
@@ -223,33 +232,33 @@ static void test_log_write_read(void)
 		set_test_hash(log.value.update.new_hash, i + 1);
 
 		err = reftable_writer_add_log(w, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_ref(&it, names[N - 1]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
+	check(!err);
 
 	/* end of iteration. */
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT(0 < err);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_ref_record_release(&ref);
@@ -257,23 +266,21 @@ static void test_log_write_read(void)
 	reftable_reader_init_log_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	i = 0;
 	while (1) {
 		int err = reftable_iterator_next_log(&it, &log);
-		if (err > 0) {
+		if (err > 0)
 			break;
-		}
-
-		EXPECT_ERR(err);
-		EXPECT_STREQ(names[i], log.refname);
-		EXPECT(i == log.update_index);
+		check(!err);
+		check_str(names[i], log.refname);
+		check_int(i, ==, log.update_index);
 		i++;
 		reftable_log_record_release(&log);
 	}
 
-	EXPECT(i == N);
+	check_int(i, ==, N);
 	reftable_iterator_destroy(&it);
 
 	/* cleanup. */
@@ -282,7 +289,7 @@ static void test_log_write_read(void)
 	reader_close(&rd);
 }
 
-static void test_log_zlib_corruption(void)
+static void t_log_zlib_corruption(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 256,
@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_log_iterator(&rd, &it);
 	err = reftable_iterator_seek_log(&it, "refname");
-	EXPECT(err == REFTABLE_ZLIB_ERROR);
+	check_int(err, ==, REFTABLE_ZLIB_ERROR);
 
 	reftable_iterator_destroy(&it);
 
@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)
 	reader_close(&rd);
 }
 
-static void test_table_read_write_sequential(void)
+static void t_table_read_write_sequential(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_iterator it = { NULL };
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err = 0;
 	int j = 0;
 
@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	while (1) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(r >= 0);
-		if (r > 0) {
+		check_int(r, >=, 0);
+		if (r > 0)
 			break;
-		}
-		EXPECT(0 == strcmp(names[j], ref.refname));
-		EXPECT(update_index == ref.update_index);
+		check_str(names[j], ref.refname);
+		check_int(update_index, ==, ref.update_index);
 
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == N);
+	check_int(j, ==, N);
 	reftable_iterator_destroy(&it);
 	strbuf_release(&buf);
 	free_names(names);
@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)
 	reader_close(&rd);
 }
 
-static void test_table_write_small_table(void)
+static void t_table_write_small_table(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 1;
 	write_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);
-	EXPECT(buf.len < 200);
+	check_int(buf.len, <, 200);
 	strbuf_release(&buf);
 	free_names(names);
 }
 
-static void test_table_read_api(void)
+static void t_table_read_api(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i;
-	struct reftable_log_record log = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_log_record log = { 0 };
+	struct reftable_iterator it = { 0 };
 
 	write_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, names[0]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_log(&it, &log);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_iterator_destroy(&it);
 	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
 
-static void test_table_read_write_seek(int index, int hash_id)
+static void t_table_read_write_seek(int index, int hash_id)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i = 0;
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	struct strbuf pastLast = STRBUF_INIT;
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 
 	write_table(&names, &buf, N, 256, hash_id);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	EXPECT(hash_id == reftable_reader_hash_id(&rd));
+	check(!err);
+	check_int(hash_id, ==, reftable_reader_hash_id(&rd));
 
-	if (!index) {
+	if (!index)
 		rd.ref_offsets.index_offset = 0;
-	} else {
-		EXPECT(rd.ref_offsets.index_offset > 0);
-	}
+	else
+		check_int(rd.ref_offsets.index_offset, >, 0);
 
 	for (i = 1; i < N; i++) {
 		reftable_reader_init_ref_iterator(&rd, &it);
 		err = reftable_iterator_seek_ref(&it, names[i]);
-		EXPECT_ERR(err);
+		check(!err);
 		err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT_ERR(err);
-		EXPECT(0 == strcmp(names[i], ref.refname));
-		EXPECT(REFTABLE_REF_VAL1 == ref.value_type);
-		EXPECT(i == ref.value.val1[0]);
+		check(!err);
+		check_str(names[i], ref.refname);
+		check_int(REFTABLE_REF_VAL1, ==, ref.value_type);
+		check_int(i, ==, ref.value.val1[0]);
 
 		reftable_ref_record_release(&ref);
 		reftable_iterator_destroy(&it);
@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, pastLast.buf);
 	if (err == 0) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	} else {
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	}
 
 	strbuf_release(&pastLast);
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_free(names);
 	reader_close(&rd);
 }
 
-static void test_table_read_write_seek_linear(void)
+static void t_table_read_write_seek_linear(void)
 {
-	test_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_linear_sha256(void)
+static void t_table_read_write_seek_linear_sha256(void)
 {
-	test_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_index(void)
+static void t_table_read_write_seek_index(void)
 {
-	test_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_refs_for(int indexed)
+static void t_table_refs_for(int indexed)
 {
 	int N = 50;
 	char **want_names = reftable_calloc(N + 1, sizeof(*want_names));
@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
 	int n;
 	int err;
 	struct reftable_reader rd;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int j;
 
 	set_test_hash(want_hash, 4);
@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)
 		uint8_t hash[GIT_SHA1_RAWSZ];
 		char fill[51] = { 0 };
 		char name[100];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 
 		memset(hash, i, sizeof(hash));
 		memset(fill, 'x', 50);
@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)
 		 */
 		/* blocks. */
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		if (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||
-		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {
+		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))
 			want_names[want_names_len++] = xstrdup(name);
-		}
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	reftable_writer_free(w);
 	w = NULL;
@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	if (!indexed) {
+	check(!err);
+	if (!indexed)
 		rd.obj_offsets.is_present = 0;
-	}
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_iterator_destroy(&it);
 
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
-	EXPECT_ERR(err);
+	check(!err);
 
 	j = 0;
 	while (1) {
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err >= 0);
-		if (err > 0) {
+		check_int(err, >=, 0);
+		if (err > 0)
 			break;
-		}
-
-		EXPECT(j < want_names_len);
-		EXPECT(0 == strcmp(ref.refname, want_names[j]));
+		check_int(j, <, want_names_len);
+		check_str(ref.refname, want_names[j]);
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == want_names_len);
+	check_int(j, ==, want_names_len);
 
 	strbuf_release(&buf);
 	free_names(want_names);
@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)
 	reader_close(&rd);
 }
 
-static void test_table_refs_for_no_index(void)
+static void t_table_refs_for_no_index(void)
 {
-	test_table_refs_for(0);
+	t_table_refs_for(0);
 }
 
-static void test_table_refs_for_obj_index(void)
+static void t_table_refs_for_obj_index(void)
 {
-	test_table_refs_for(1);
+	t_table_refs_for(1);
 }
 
-static void test_write_empty_table(void)
+static void t_write_empty_table(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_reader *rd = NULL;
-	struct reftable_ref_record rec = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record rec = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 
-	EXPECT(buf.len == header_size(1) + footer_size(1));
+	check_int(buf.len, ==, header_size(1) + footer_size(1));
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &rec);
-	EXPECT(err > 0);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_reader_free(rd);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_min_length(void)
+static void t_write_object_id_min_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)
 		snprintf(name, sizeof(name), "ref%05d", i);
 		ref.refname = name;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 2);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 2);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_length(void)
+static void t_write_object_id_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)
 		ref.refname = name;
 		ref.value.val1[15] = i;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 16);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 16);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_empty_key(void)
+static void t_write_empty_key(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -747,15 +746,15 @@ static void test_write_empty_key(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &ref);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_key_order(void)
+static void t_write_key_order(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -782,15 +781,15 @@ static void test_write_key_order(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &refs[0]);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_multiple_indices(void)
+static void t_write_multiple_indices(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	for (i = 0; i < 100; i++) {
@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)
 		log.refname = buf.buf,
 
 		err = reftable_writer_add_log(writer, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	reftable_writer_close(writer);
@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)
 	 * for each of the block types.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.index_offset > 0);
-	EXPECT(stats->obj_stats.index_offset > 0);
-	EXPECT(stats->log_stats.index_offset > 0);
+	check_int(stats->ref_stats.index_offset, >, 0);
+	check_int(stats->obj_stats.index_offset, >, 0);
+	check_int(stats->log_stats.index_offset, >, 0);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the log uses the log index now. In case there is any
@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)
 	 */
 	reftable_reader_init_log_iterator(reader, &it);
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)
 	strbuf_release(&buf);
 }
 
-static void test_write_multi_level_index(void)
+static void t_write_multi_level_index(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	reftable_writer_close(writer);
 
@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)
 	 * multi-level index.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.max_index_level == 2);
+	check_int(stats->ref_stats.max_index_level, ==, 2);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the last ref should work as expected.
 	 */
 	reftable_reader_init_ref_iterator(reader, &it);
 	err = reftable_iterator_seek_ref(&it, "refs/heads/199");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)
 	strbuf_release(&buf);
 }
 
-static void test_corrupt_table_empty(void)
+static void t_corrupt_table_empty(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 }
 
-static void test_corrupt_table(void)
+static void t_corrupt_table(void)
 {
 	uint8_t zeros[1024] = { 0 };
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 	strbuf_add(&buf, zeros, sizeof(zeros));
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	strbuf_release(&buf);
 }
 
-int readwrite_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_log_zlib_corruption);
-	RUN_TEST(test_corrupt_table);
-	RUN_TEST(test_corrupt_table_empty);
-	RUN_TEST(test_log_write_read);
-	RUN_TEST(test_write_key_order);
-	RUN_TEST(test_table_read_write_seek_linear_sha256);
-	RUN_TEST(test_log_buffer_size);
-	RUN_TEST(test_table_write_small_table);
-	RUN_TEST(test_buffer);
-	RUN_TEST(test_table_read_api);
-	RUN_TEST(test_table_read_write_sequential);
-	RUN_TEST(test_table_read_write_seek_linear);
-	RUN_TEST(test_table_read_write_seek_index);
-	RUN_TEST(test_table_refs_for_no_index);
-	RUN_TEST(test_table_refs_for_obj_index);
-	RUN_TEST(test_write_empty_key);
-	RUN_TEST(test_write_empty_table);
-	RUN_TEST(test_log_overflow);
-	RUN_TEST(test_write_object_id_length);
-	RUN_TEST(test_write_object_id_min_length);
-	RUN_TEST(test_write_multiple_indices);
-	RUN_TEST(test_write_multi_level_index);
-	return 0;
+	TEST(t_buffer(), "strbuf works as blocksource");
+	TEST(t_corrupt_table(), "read-write on corrupted table");
+	TEST(t_corrupt_table_empty(), "read-write on an empty table");
+	TEST(t_log_buffer_size(), "buffer extension for log compression");
+	TEST(t_log_overflow(), "log overflow returns expected error");
+	TEST(t_log_write_read(), "read-write on log records");
+	TEST(t_log_zlib_corruption(), "reading corrupted log record returns expected error");
+	TEST(t_table_read_api(), "read on a table");
+	TEST(t_table_read_write_seek_index(), "read-write on a table with index");
+	TEST(t_table_read_write_seek_linear(), "read-write on a table without index (SHA1)");
+	TEST(t_table_read_write_seek_linear_sha256(), "read-write on a table without index (SHA256)");
+	TEST(t_table_read_write_sequential(), "sequential read-write on a table");
+	TEST(t_table_refs_for_no_index(), "refs-only table with no index");
+	TEST(t_table_refs_for_obj_index(), "refs-only table with index");
+	TEST(t_table_write_small_table(), "write_table works");
+	TEST(t_write_empty_key(), "write on refs with empty keys");
+	TEST(t_write_empty_table(), "read-write on empty tables");
+	TEST(t_write_key_order(), "refs must be written in increasing order");
+	TEST(t_write_multi_level_index(), "table with multi-level index");
+	TEST(t_write_multiple_indices(), "table with indices for multiple block types");
+	TEST(t_write_object_id_length(), "prefix compression on writing refs");
+	TEST(t_write_object_id_min_length(), "prefix compression on writing refs");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH 2/5] t-reftable-readwrite: use free_names() instead of a for loop

[Chandra Pratap]

free_names() as defined by reftable/basics.{c,h} frees a NULL
terminated array of malloced strings along with the array itself.
Use this function instead of a for loop to free such an array.

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-readwrite.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 235e3d94c7..e90f2bf9de 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -413,7 +413,6 @@ static void t_table_read_api(void)
 	struct reftable_reader rd = { 0 };
 	struct reftable_block_source source = { 0 };
 	int err;
-	int i;
 	struct reftable_log_record log = { 0 };
 	struct reftable_iterator it = { 0 };
 
@@ -432,10 +431,8 @@ static void t_table_read_api(void)
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+	free_names(names);
 	reftable_iterator_destroy(&it);
-	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
@@ -498,9 +495,7 @@ static void t_table_read_write_seek(int index, int hash_id)
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
-	reftable_free(names);
+	free_names(names);
 	reader_close(&rd);
 }
 
-- 
2.45.GIT

[PATCH 3/5] t-reftable-readwrite: use 'for' in place of infinite 'while' loops

[Chandra Pratap]

Using a for loop with an empty conditional statement is more concise
and easier to read than an infinite 'while' loop in instances
where we need a loop variable. Hence, replace such instances of a
'while' loop with the equivalent 'for' loop.

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-readwrite.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index e90f2bf9de..7daf28ec6d 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -268,15 +268,13 @@ static void t_log_write_read(void)
 	err = reftable_iterator_seek_log(&it, "");
 	check(!err);
 
-	i = 0;
-	while (1) {
+	for (i = 0; ; i++) {
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
 		check(!err);
 		check_str(names[i], log.refname);
 		check_int(i, ==, log.update_index);
-		i++;
 		reftable_log_record_release(&log);
 	}
 
@@ -374,7 +372,7 @@ static void t_table_read_write_sequential(void)
 	err = reftable_iterator_seek_ref(&it, "");
 	check(!err);
 
-	while (1) {
+	for (j = 0; ; j++) {
 		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
 		check_int(r, >=, 0);
@@ -382,8 +380,6 @@ static void t_table_read_write_sequential(void)
 			break;
 		check_str(names[j], ref.refname);
 		check_int(update_index, ==, ref.update_index);
-
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, N);
@@ -589,15 +585,13 @@ static void t_table_refs_for(int indexed)
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
 	check(!err);
 
-	j = 0;
-	while (1) {
+	for (j = 0; ; j++) {
 		int err = reftable_iterator_next_ref(&it, &ref);
 		check_int(err, >=, 0);
 		if (err > 0)
 			break;
 		check_int(j, <, want_names_len);
 		check_str(ref.refname, want_names[j]);
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, want_names_len);
-- 
2.45.GIT

[PATCH 4/5] t-reftable-readwrite: add test for known error

[Chandra Pratap]

When using reftable_writer_add_ref() to add a ref record to a
reftable writer, The update_index of the ref record must be within
the limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR
is returned. This scenario is currently left untested. Add a test
case 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-readwrite.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 7daf28ec6d..a5462441d3 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -773,6 +773,11 @@ static void t_write_key_order(void)
 	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
 	check_int(err, ==, REFTABLE_API_ERROR);
+
+	refs[0].update_index = 2;
+	err = reftable_writer_add_ref(w, &refs[0]);
+	check_int(err, ==, REFTABLE_API_ERROR);
+
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
-- 
2.45.GIT

[PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Chandra Pratap]

reftable/reftable-reader.h lists two print functions useful in
debugging, reftable_reader_print_file() and
reftable_reader_print_blocks(). As of now, both these functions
are left unexercised by all of the reftable tests. Add a test
function to exercise both these functions. This has the added
benefit of testing reftable_block_source_from_file(), which
currently remains untested as well.

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-readwrite.c | 75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index a5462441d3..8c6f2f1f5d 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -11,6 +11,8 @@ license that can be found in the LICENSE file or at
 #include "reftable/blocksource.h"
 #include "reftable/reftable-error.h"
 #include "reftable/reftable-writer.h"
+#include "tempfile.h"
+#include "write-or-die.h"
 
 static const int update_index = 5;
 
@@ -25,11 +27,23 @@ static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
 	return sz;
 }
 
+static ssize_t fd_write(void *b, const void *data, size_t sz)
+{
+	int *fdp = (int *)b;
+	return write_in_full(*fdp, data, sz);
+}
+
 static int noop_flush(void *arg)
 {
 	return 0;
 }
 
+static int fd_flush(void *arg)
+{
+	int *fdp = (int *)arg;
+	return fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);
+}
+
 static void t_buffer(void)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -944,6 +958,66 @@ static void t_corrupt_table(void)
 	strbuf_release(&buf);
 }
 
+static void t_table_print(void)
+{
+	char name[100];
+	struct reftable_write_options opts = {
+		.block_size = 512,
+		.hash_id = GIT_SHA1_FORMAT_ID,
+	};
+	struct reftable_ref_record ref = { 0 };
+	struct reftable_log_record log = { 0 };
+	struct reftable_writer *w = NULL;
+	struct tempfile *tmp = NULL;
+	size_t i, N = 3;
+	int n, fd;
+
+	xsnprintf(name, sizeof(name), "t-reftable-readwrite-%d-XXXXXX", __LINE__);
+	tmp = mks_tempfile_t(name);
+	fd = get_tempfile_fd(tmp);
+	w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);
+	reftable_writer_set_limits(w, 0, update_index);
+
+	for (i = 0; i < N; i++) {
+		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
+		ref.refname = name;
+		ref.update_index = i;
+		ref.value_type = REFTABLE_REF_VAL1;
+		set_test_hash(ref.value.val1, i);
+
+		n = reftable_writer_add_ref(w, &ref);
+		check_int(n, ==, 0);
+	}
+
+	for (i = 0; i < N; i++) {
+		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
+		log.refname = name;
+		log.update_index = i;
+		log.value_type = REFTABLE_LOG_UPDATE;
+		set_test_hash(log.value.update.new_hash, i);
+		log.value.update.name = (char *) "John Doe";
+		log.value.update.email = (char *) "johndoe@anon.org";
+		log.value.update.time = 0x6673e5b9;
+		log.value.update.message = (char *) "message";
+
+		n = reftable_writer_add_log(w, &log);
+		check_int(n, ==, 0);
+	}
+
+	n = reftable_writer_close(w);
+	check_int(n, ==, 0);
+
+	test_msg("testing printing functionality:");
+	n = reftable_reader_print_file(tmp->filename.buf);
+	check_int(n, ==, 0);
+	n = reftable_reader_print_blocks(tmp->filename.buf);
+	/* end of blocks is denoted by a return value of 1 */
+	check_int(n, ==, 1);
+
+	delete_tempfile(&tmp);
+	reftable_writer_free(w);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
 	TEST(t_buffer(), "strbuf works as blocksource");
@@ -953,6 +1027,7 @@ int cmd_main(int argc, const char *argv[])
 	TEST(t_log_overflow(), "log overflow returns expected error");
 	TEST(t_log_write_read(), "read-write on log records");
 	TEST(t_log_zlib_corruption(), "reading corrupted log record returns expected error");
+	TEST(t_table_print(), "print tables and blocks");
 	TEST(t_table_read_api(), "read on a table");
 	TEST(t_table_read_write_seek_index(), "read-write on a table with index");
 	TEST(t_table_read_write_seek_linear(), "read-write on a table without index (SHA1)");
-- 
2.45.GIT

Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:
> +static void t_table_print(void)
> +{
> +	char name[100];
> +	struct reftable_write_options opts = {
> +		.block_size = 512,
> +		.hash_id = GIT_SHA1_FORMAT_ID,
> +	};
> +	struct reftable_ref_record ref = { 0 };
> +	struct reftable_log_record log = { 0 };
> +	struct reftable_writer *w = NULL;
> +	struct tempfile *tmp = NULL;
> +	size_t i, N = 3;
> +	int n, fd;
> +
> +	xsnprintf(name, sizeof(name), "t-reftable-readwrite-%d-XXXXXX", __LINE__);

Is it really required to include the line number in this file? This
feels unnecessarily defensive to me as `mks_tempfile_t()` should already
make sure that we get a unique filename. So if we drop that, we could
skip this call to `xsnprintf()`.

> +	tmp = mks_tempfile_t(name);
> +	fd = get_tempfile_fd(tmp);
> +	w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);
> +	reftable_writer_set_limits(w, 0, update_index);
> +
> +	for (i = 0; i < N; i++) {
> +		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> +		ref.refname = name;
> +		ref.update_index = i;
> +		ref.value_type = REFTABLE_REF_VAL1;
> +		set_test_hash(ref.value.val1, i);
> +
> +		n = reftable_writer_add_ref(w, &ref);
> +		check_int(n, ==, 0);
> +	}
> +
> +	for (i = 0; i < N; i++) {
> +		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> +		log.refname = name;
> +		log.update_index = i;
> +		log.value_type = REFTABLE_LOG_UPDATE;
> +		set_test_hash(log.value.update.new_hash, i);
> +		log.value.update.name = (char *) "John Doe";
> +		log.value.update.email = (char *) "johndoe@anon.org";
> +		log.value.update.time = 0x6673e5b9;
> +		log.value.update.message = (char *) "message";
> +
> +		n = reftable_writer_add_log(w, &log);
> +		check_int(n, ==, 0);
> +	}
> +
> +	n = reftable_writer_close(w);
> +	check_int(n, ==, 0);
> +
> +	test_msg("testing printing functionality:");

Is it intentionally that this line still exists? If so, I think it
really only causes unnecessary noise and should rather be dropped.

> +	n = reftable_reader_print_file(tmp->filename.buf);
> +	check_int(n, ==, 0);

Wait, doesn't this print to stdout? I don't think it is a good idea to
exercise the function as-is. For one, it would pollute stdout with data
that we shouldn't care about. Second, it doesn't verify that the result
is actually what we expect.

I can see two options:

  1. Refactor these interfaces such that they take a file descriptor as
     input that they are writing to. This would allow us to exercise
     that the output is correct.

  2. Rip out this function. I don't think this functionality should be
     part of the library in the first place, and it really only exists
     because of "reftable/dump.c".

I think the latter is the better option. The functionality exists to
drive `cmd__dump_reftable()` in our reftable test helper. We should
likely make the whole implementation of this an internal implementation
detail and not expose it.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]

On Thu, Aug 08, 2024 at 10:12:07AM +0200, Patrick Steinhardt wrote:
> On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:
> > +static void t_table_print(void)
> > +{
> > +	char name[100];
> > +	struct reftable_write_options opts = {
> > +		.block_size = 512,
> > +		.hash_id = GIT_SHA1_FORMAT_ID,
> > +	};
> > +	struct reftable_ref_record ref = { 0 };
> > +	struct reftable_log_record log = { 0 };
> > +	struct reftable_writer *w = NULL;
> > +	struct tempfile *tmp = NULL;
> > +	size_t i, N = 3;
> > +	int n, fd;
> > +
> > +	xsnprintf(name, sizeof(name), "t-reftable-readwrite-%d-XXXXXX", __LINE__);
> 
> Is it really required to include the line number in this file? This
> feels unnecessarily defensive to me as `mks_tempfile_t()` should already
> make sure that we get a unique filename. So if we drop that, we could
> skip this call to `xsnprintf()`.
> 
> > +	tmp = mks_tempfile_t(name);
> > +	fd = get_tempfile_fd(tmp);
> > +	w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);
> > +	reftable_writer_set_limits(w, 0, update_index);
> > +
> > +	for (i = 0; i < N; i++) {
> > +		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > +		ref.refname = name;
> > +		ref.update_index = i;
> > +		ref.value_type = REFTABLE_REF_VAL1;
> > +		set_test_hash(ref.value.val1, i);
> > +
> > +		n = reftable_writer_add_ref(w, &ref);
> > +		check_int(n, ==, 0);
> > +	}
> > +
> > +	for (i = 0; i < N; i++) {
> > +		xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > +		log.refname = name;
> > +		log.update_index = i;
> > +		log.value_type = REFTABLE_LOG_UPDATE;
> > +		set_test_hash(log.value.update.new_hash, i);
> > +		log.value.update.name = (char *) "John Doe";
> > +		log.value.update.email = (char *) "johndoe@anon.org";
> > +		log.value.update.time = 0x6673e5b9;
> > +		log.value.update.message = (char *) "message";
> > +
> > +		n = reftable_writer_add_log(w, &log);
> > +		check_int(n, ==, 0);
> > +	}
> > +
> > +	n = reftable_writer_close(w);
> > +	check_int(n, ==, 0);
> > +
> > +	test_msg("testing printing functionality:");
> 
> Is it intentionally that this line still exists? If so, I think it
> really only causes unnecessary noise and should rather be dropped.
> 
> > +	n = reftable_reader_print_file(tmp->filename.buf);
> > +	check_int(n, ==, 0);
> 
> Wait, doesn't this print to stdout? I don't think it is a good idea to
> exercise the function as-is. For one, it would pollute stdout with data
> that we shouldn't care about. Second, it doesn't verify that the result
> is actually what we expect.
> 
> I can see two options:
> 
>   1. Refactor these interfaces such that they take a file descriptor as
>      input that they are writing to. This would allow us to exercise
>      that the output is correct.
> 
>   2. Rip out this function. I don't think this functionality should be
>      part of the library in the first place, and it really only exists
>      because of "reftable/dump.c".
> 
> I think the latter is the better option. The functionality exists to
> drive `cmd__dump_reftable()` in our reftable test helper. We should
> likely make the whole implementation of this an internal implementation
> detail and not expose it.

For the record: I've got a bigger patch series in development that drops
the generic reftable interfaces. As part of this, I'll also rip out the
functionality provided by "reftabel/dump.c".

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Chandra Pratap]

On Thu, 8 Aug 2024 at 17:36, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Aug 08, 2024 at 10:12:07AM +0200, Patrick Steinhardt wrote:
> > On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:
> > > +static void t_table_print(void)
> > > +{
> > > +   char name[100];
> > > +   struct reftable_write_options opts = {
> > > +           .block_size = 512,
> > > +           .hash_id = GIT_SHA1_FORMAT_ID,
> > > +   };
> > > +   struct reftable_ref_record ref = { 0 };
> > > +   struct reftable_log_record log = { 0 };
> > > +   struct reftable_writer *w = NULL;
> > > +   struct tempfile *tmp = NULL;
> > > +   size_t i, N = 3;
> > > +   int n, fd;
> > > +
> > > +   xsnprintf(name, sizeof(name), "t-reftable-readwrite-%d-XXXXXX", __LINE__);
> >
> > Is it really required to include the line number in this file? This
> > feels unnecessarily defensive to me as `mks_tempfile_t()` should already
> > make sure that we get a unique filename. So if we drop that, we could
> > skip this call to `xsnprintf()`.
> >
> > > +   tmp = mks_tempfile_t(name);
> > > +   fd = get_tempfile_fd(tmp);
> > > +   w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);
> > > +   reftable_writer_set_limits(w, 0, update_index);
> > > +
> > > +   for (i = 0; i < N; i++) {
> > > +           xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > > +           ref.refname = name;
> > > +           ref.update_index = i;
> > > +           ref.value_type = REFTABLE_REF_VAL1;
> > > +           set_test_hash(ref.value.val1, i);
> > > +
> > > +           n = reftable_writer_add_ref(w, &ref);
> > > +           check_int(n, ==, 0);
> > > +   }
> > > +
> > > +   for (i = 0; i < N; i++) {
> > > +           xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > > +           log.refname = name;
> > > +           log.update_index = i;
> > > +           log.value_type = REFTABLE_LOG_UPDATE;
> > > +           set_test_hash(log.value.update.new_hash, i);
> > > +           log.value.update.name = (char *) "John Doe";
> > > +           log.value.update.email = (char *) "johndoe@anon.org";
> > > +           log.value.update.time = 0x6673e5b9;
> > > +           log.value.update.message = (char *) "message";
> > > +
> > > +           n = reftable_writer_add_log(w, &log);
> > > +           check_int(n, ==, 0);
> > > +   }
> > > +
> > > +   n = reftable_writer_close(w);
> > > +   check_int(n, ==, 0);
> > > +
> > > +   test_msg("testing printing functionality:");
> >
> > Is it intentionally that this line still exists? If so, I think it
> > really only causes unnecessary noise and should rather be dropped.
> >
> > > +   n = reftable_reader_print_file(tmp->filename.buf);
> > > +   check_int(n, ==, 0);
> >
> > Wait, doesn't this print to stdout? I don't think it is a good idea to
> > exercise the function as-is. For one, it would pollute stdout with data
> > that we shouldn't care about. Second, it doesn't verify that the result
> > is actually what we expect.
> >
> > I can see two options:
> >
> >   1. Refactor these interfaces such that they take a file descriptor as
> >      input that they are writing to. This would allow us to exercise
> >      that the output is correct.
> >
> >   2. Rip out this function. I don't think this functionality should be
> >      part of the library in the first place, and it really only exists
> >      because of "reftable/dump.c".
> >
> > I think the latter is the better option. The functionality exists to
> > drive `cmd__dump_reftable()` in our reftable test helper. We should
> > likely make the whole implementation of this an internal implementation
> > detail and not expose it.
>
> For the record: I've got a bigger patch series in development that drops
> the generic reftable interfaces. As part of this, I'll also rip out the
> functionality provided by "reftabel/dump.c".

Cool, I'll just drop this patch from the series then.

[GSoC][PATCH v2 0/4] t: port reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/readwrite_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 v2:
- Drop the fifth patch (t-reftable-readwrite: add tests for print
  functions) of the previous series

CI/PR: https://github.com/gitgitgadget/git/pull/1770

Chandra Pratap(4):
t: move reftable/readwrite_test.c to the unit testing framework
t-reftable-readwrite: use free_names() instead of a for loop
t-reftable-readwrite: use 'for' in place of infinite 'while' loops
t-reftable-readwrite: add test for known error

Makefile                                                         |   2 +-
reftable/reftable-tests.h                                        |   1 -
t/helper/test-reftable.c                                         |   1 -
reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 440 +++++++++++++++++++++++-----------------
4 files changed, 218 insertions(+), 226 deletions(-)

Range-diff against v1:

1:  e4b01cd856 < -:  ---------- t-reftable-readwrite: add tests for print functions

[PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

reftable/readwrite_test.c exercises the functions defined in
reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
reftable/readwrite_test.c to the unit testing framework. Migration
involves refactoring the tests to use the unit testing framework
instead of reftable's test framework and renaming the tests to
align with unit-tests' naming conventions.

Since some tests in reftable/readwrite_test.c use the functions
set_test_hash(), noop_flush() and strbuf_add_void() defined in
reftable/test_framework.{c,h} but these files are not #included
in the ported unit test, copy these functions in the new test file.

While at it, ensure structs are 0-initialized with '= { 0 }'
instead of '= { NULL }'.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 reftable/reftable-tests.h                     |   1 -
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------
 4 files changed, 210 insertions(+), 212 deletions(-)
 rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)

diff --git a/Makefile b/Makefile
index 3863e60b66..76e4d1c1ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-merged
+UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..7d955393d2 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);
 int block_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
-int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9d378427da..d371e9f9dd 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	block_test_main(argc, argv);
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
-	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c
similarity index 70%
rename from reftable/readwrite_test.c
rename to t/unit-tests/t-reftable-readwrite.c
index f411abfe9c..235e3d94c7 100644
--- a/reftable/readwrite_test.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-
-#include "basics.h"
-#include "block.h"
-#include "blocksource.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-#include "reftable-writer.h"
+#include "test-lib.h"
+#include "reftable/reader.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-writer.h"
 
 static const int update_index = 5;
 
-static void test_buffer(void)
+static void set_test_hash(uint8_t *p, int i)
+{
+	memset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));
+}
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
+
+static int noop_flush(void *arg)
+{
+	return 0;
+}
+
+static void t_buffer(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_block out = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_block out = { 0 };
 	int n;
 	uint8_t in[] = "hello";
 	strbuf_add(&buf, in, sizeof(in));
 	block_source_from_strbuf(&source, &buf);
-	EXPECT(block_source_size(&source) == 6);
+	check_int(block_source_size(&source), ==, 6);
 	n = block_source_read_block(&source, &out, 0, sizeof(in));
-	EXPECT(n == sizeof(in));
-	EXPECT(!memcmp(in, out.data, n));
+	check_int(n, ==, sizeof(in));
+	check(!memcmp(in, out.data, n));
 	reftable_block_done(&out);
 
 	n = block_source_read_block(&source, &out, 1, 2);
-	EXPECT(n == 2);
-	EXPECT(!memcmp(out.data, "el", 2));
+	check_int(n, ==, 2);
+	check(!memcmp(out.data, "el", 2));
 
 	reftable_block_done(&out);
 	block_source_close(&source);
@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 	};
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0, n;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	const struct reftable_stats *stats = NULL;
 
 	REFTABLE_CALLOC_ARRAY(*names, N + 1);
@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		(*names)[i] = xstrdup(name);
 
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	for (i = 0; i < N; i++) {
@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		log.value.update.message = (char *) "message";
 
 		n = reftable_writer_add_log(w, &log);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
 	for (i = 0; i < stats->ref_stats.blocks; i++) {
 		int off = i * opts.block_size;
-		if (off == 0) {
-			off = header_size(
-				(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
-		}
-		EXPECT(buf->buf[off] == 'r');
+		if (!off)
+			off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
+		check(buf->buf[off] == 'r');
 	}
 
-	EXPECT(stats->log_stats.blocks > 0);
+	check(stats->log_stats.blocks > 0);
 	reftable_writer_free(w);
 }
 
-static void test_log_buffer_size(void)
+static void t_log_buffer_size(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_write_options opts = {
@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)
 	}
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_overflow(void)
+static void t_log_overflow(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	char msg[256] = { 0 };
@@ -177,12 +186,12 @@ static void test_log_overflow(void)
 	memset(msg, 'x', sizeof(msg) - 1);
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);
+	check_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_write_read(void)
+static void t_log_write_read(void)
 {
 	int N = 2;
 	char **names = reftable_calloc(N + 1, sizeof(*names));
@@ -190,13 +199,13 @@ static void test_log_write_read(void)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	int n;
-	struct reftable_iterator it = { NULL };
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
@@ -204,17 +213,17 @@ static void test_log_write_read(void)
 	reftable_writer_set_limits(w, 0, N);
 	for (i = 0; i < N; i++) {
 		char name[256];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		snprintf(name, sizeof(name), "b%02d%0*d", i, 130, 7);
 		names[i] = xstrdup(name);
 		ref.refname = name;
 		ref.update_index = i;
 
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	for (i = 0; i < N; i++) {
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 
 		log.refname = names[i];
 		log.update_index = i;
@@ -223,33 +232,33 @@ static void test_log_write_read(void)
 		set_test_hash(log.value.update.new_hash, i + 1);
 
 		err = reftable_writer_add_log(w, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_ref(&it, names[N - 1]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
+	check(!err);
 
 	/* end of iteration. */
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT(0 < err);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_ref_record_release(&ref);
@@ -257,23 +266,21 @@ static void test_log_write_read(void)
 	reftable_reader_init_log_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	i = 0;
 	while (1) {
 		int err = reftable_iterator_next_log(&it, &log);
-		if (err > 0) {
+		if (err > 0)
 			break;
-		}
-
-		EXPECT_ERR(err);
-		EXPECT_STREQ(names[i], log.refname);
-		EXPECT(i == log.update_index);
+		check(!err);
+		check_str(names[i], log.refname);
+		check_int(i, ==, log.update_index);
 		i++;
 		reftable_log_record_release(&log);
 	}
 
-	EXPECT(i == N);
+	check_int(i, ==, N);
 	reftable_iterator_destroy(&it);
 
 	/* cleanup. */
@@ -282,7 +289,7 @@ static void test_log_write_read(void)
 	reader_close(&rd);
 }
 
-static void test_log_zlib_corruption(void)
+static void t_log_zlib_corruption(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 256,
@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_log_iterator(&rd, &it);
 	err = reftable_iterator_seek_log(&it, "refname");
-	EXPECT(err == REFTABLE_ZLIB_ERROR);
+	check_int(err, ==, REFTABLE_ZLIB_ERROR);
 
 	reftable_iterator_destroy(&it);
 
@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)
 	reader_close(&rd);
 }
 
-static void test_table_read_write_sequential(void)
+static void t_table_read_write_sequential(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_iterator it = { NULL };
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err = 0;
 	int j = 0;
 
@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	while (1) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(r >= 0);
-		if (r > 0) {
+		check_int(r, >=, 0);
+		if (r > 0)
 			break;
-		}
-		EXPECT(0 == strcmp(names[j], ref.refname));
-		EXPECT(update_index == ref.update_index);
+		check_str(names[j], ref.refname);
+		check_int(update_index, ==, ref.update_index);
 
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == N);
+	check_int(j, ==, N);
 	reftable_iterator_destroy(&it);
 	strbuf_release(&buf);
 	free_names(names);
@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)
 	reader_close(&rd);
 }
 
-static void test_table_write_small_table(void)
+static void t_table_write_small_table(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 1;
 	write_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);
-	EXPECT(buf.len < 200);
+	check_int(buf.len, <, 200);
 	strbuf_release(&buf);
 	free_names(names);
 }
 
-static void test_table_read_api(void)
+static void t_table_read_api(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i;
-	struct reftable_log_record log = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_log_record log = { 0 };
+	struct reftable_iterator it = { 0 };
 
 	write_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, names[0]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_log(&it, &log);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_iterator_destroy(&it);
 	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
 
-static void test_table_read_write_seek(int index, int hash_id)
+static void t_table_read_write_seek(int index, int hash_id)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i = 0;
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	struct strbuf pastLast = STRBUF_INIT;
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 
 	write_table(&names, &buf, N, 256, hash_id);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	EXPECT(hash_id == reftable_reader_hash_id(&rd));
+	check(!err);
+	check_int(hash_id, ==, reftable_reader_hash_id(&rd));
 
-	if (!index) {
+	if (!index)
 		rd.ref_offsets.index_offset = 0;
-	} else {
-		EXPECT(rd.ref_offsets.index_offset > 0);
-	}
+	else
+		check_int(rd.ref_offsets.index_offset, >, 0);
 
 	for (i = 1; i < N; i++) {
 		reftable_reader_init_ref_iterator(&rd, &it);
 		err = reftable_iterator_seek_ref(&it, names[i]);
-		EXPECT_ERR(err);
+		check(!err);
 		err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT_ERR(err);
-		EXPECT(0 == strcmp(names[i], ref.refname));
-		EXPECT(REFTABLE_REF_VAL1 == ref.value_type);
-		EXPECT(i == ref.value.val1[0]);
+		check(!err);
+		check_str(names[i], ref.refname);
+		check_int(REFTABLE_REF_VAL1, ==, ref.value_type);
+		check_int(i, ==, ref.value.val1[0]);
 
 		reftable_ref_record_release(&ref);
 		reftable_iterator_destroy(&it);
@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, pastLast.buf);
 	if (err == 0) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	} else {
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	}
 
 	strbuf_release(&pastLast);
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_free(names);
 	reader_close(&rd);
 }
 
-static void test_table_read_write_seek_linear(void)
+static void t_table_read_write_seek_linear(void)
 {
-	test_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_linear_sha256(void)
+static void t_table_read_write_seek_linear_sha256(void)
 {
-	test_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_index(void)
+static void t_table_read_write_seek_index(void)
 {
-	test_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_refs_for(int indexed)
+static void t_table_refs_for(int indexed)
 {
 	int N = 50;
 	char **want_names = reftable_calloc(N + 1, sizeof(*want_names));
@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
 	int n;
 	int err;
 	struct reftable_reader rd;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int j;
 
 	set_test_hash(want_hash, 4);
@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)
 		uint8_t hash[GIT_SHA1_RAWSZ];
 		char fill[51] = { 0 };
 		char name[100];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 
 		memset(hash, i, sizeof(hash));
 		memset(fill, 'x', 50);
@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)
 		 */
 		/* blocks. */
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		if (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||
-		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {
+		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))
 			want_names[want_names_len++] = xstrdup(name);
-		}
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	reftable_writer_free(w);
 	w = NULL;
@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	if (!indexed) {
+	check(!err);
+	if (!indexed)
 		rd.obj_offsets.is_present = 0;
-	}
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_iterator_destroy(&it);
 
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
-	EXPECT_ERR(err);
+	check(!err);
 
 	j = 0;
 	while (1) {
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err >= 0);
-		if (err > 0) {
+		check_int(err, >=, 0);
+		if (err > 0)
 			break;
-		}
-
-		EXPECT(j < want_names_len);
-		EXPECT(0 == strcmp(ref.refname, want_names[j]));
+		check_int(j, <, want_names_len);
+		check_str(ref.refname, want_names[j]);
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == want_names_len);
+	check_int(j, ==, want_names_len);
 
 	strbuf_release(&buf);
 	free_names(want_names);
@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)
 	reader_close(&rd);
 }
 
-static void test_table_refs_for_no_index(void)
+static void t_table_refs_for_no_index(void)
 {
-	test_table_refs_for(0);
+	t_table_refs_for(0);
 }
 
-static void test_table_refs_for_obj_index(void)
+static void t_table_refs_for_obj_index(void)
 {
-	test_table_refs_for(1);
+	t_table_refs_for(1);
 }
 
-static void test_write_empty_table(void)
+static void t_write_empty_table(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_reader *rd = NULL;
-	struct reftable_ref_record rec = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record rec = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 
-	EXPECT(buf.len == header_size(1) + footer_size(1));
+	check_int(buf.len, ==, header_size(1) + footer_size(1));
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &rec);
-	EXPECT(err > 0);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_reader_free(rd);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_min_length(void)
+static void t_write_object_id_min_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)
 		snprintf(name, sizeof(name), "ref%05d", i);
 		ref.refname = name;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 2);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 2);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_length(void)
+static void t_write_object_id_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)
 		ref.refname = name;
 		ref.value.val1[15] = i;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 16);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 16);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_empty_key(void)
+static void t_write_empty_key(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -747,15 +746,15 @@ static void test_write_empty_key(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &ref);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_key_order(void)
+static void t_write_key_order(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -782,15 +781,15 @@ static void test_write_key_order(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &refs[0]);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_multiple_indices(void)
+static void t_write_multiple_indices(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	for (i = 0; i < 100; i++) {
@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)
 		log.refname = buf.buf,
 
 		err = reftable_writer_add_log(writer, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	reftable_writer_close(writer);
@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)
 	 * for each of the block types.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.index_offset > 0);
-	EXPECT(stats->obj_stats.index_offset > 0);
-	EXPECT(stats->log_stats.index_offset > 0);
+	check_int(stats->ref_stats.index_offset, >, 0);
+	check_int(stats->obj_stats.index_offset, >, 0);
+	check_int(stats->log_stats.index_offset, >, 0);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the log uses the log index now. In case there is any
@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)
 	 */
 	reftable_reader_init_log_iterator(reader, &it);
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)
 	strbuf_release(&buf);
 }
 
-static void test_write_multi_level_index(void)
+static void t_write_multi_level_index(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	reftable_writer_close(writer);
 
@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)
 	 * multi-level index.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.max_index_level == 2);
+	check_int(stats->ref_stats.max_index_level, ==, 2);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the last ref should work as expected.
 	 */
 	reftable_reader_init_ref_iterator(reader, &it);
 	err = reftable_iterator_seek_ref(&it, "refs/heads/199");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)
 	strbuf_release(&buf);
 }
 
-static void test_corrupt_table_empty(void)
+static void t_corrupt_table_empty(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 }
 
-static void test_corrupt_table(void)
+static void t_corrupt_table(void)
 {
 	uint8_t zeros[1024] = { 0 };
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 	strbuf_add(&buf, zeros, sizeof(zeros));
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	strbuf_release(&buf);
 }
 
-int readwrite_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_log_zlib_corruption);
-	RUN_TEST(test_corrupt_table);
-	RUN_TEST(test_corrupt_table_empty);
-	RUN_TEST(test_log_write_read);
-	RUN_TEST(test_write_key_order);
-	RUN_TEST(test_table_read_write_seek_linear_sha256);
-	RUN_TEST(test_log_buffer_size);
-	RUN_TEST(test_table_write_small_table);
-	RUN_TEST(test_buffer);
-	RUN_TEST(test_table_read_api);
-	RUN_TEST(test_table_read_write_sequential);
-	RUN_TEST(test_table_read_write_seek_linear);
-	RUN_TEST(test_table_read_write_seek_index);
-	RUN_TEST(test_table_refs_for_no_index);
-	RUN_TEST(test_table_refs_for_obj_index);
-	RUN_TEST(test_write_empty_key);
-	RUN_TEST(test_write_empty_table);
-	RUN_TEST(test_log_overflow);
-	RUN_TEST(test_write_object_id_length);
-	RUN_TEST(test_write_object_id_min_length);
-	RUN_TEST(test_write_multiple_indices);
-	RUN_TEST(test_write_multi_level_index);
-	return 0;
+	TEST(t_buffer(), "strbuf works as blocksource");
+	TEST(t_corrupt_table(), "read-write on corrupted table");
+	TEST(t_corrupt_table_empty(), "read-write on an empty table");
+	TEST(t_log_buffer_size(), "buffer extension for log compression");
+	TEST(t_log_overflow(), "log overflow returns expected error");
+	TEST(t_log_write_read(), "read-write on log records");
+	TEST(t_log_zlib_corruption(), "reading corrupted log record returns expected error");
+	TEST(t_table_read_api(), "read on a table");
+	TEST(t_table_read_write_seek_index(), "read-write on a table with index");
+	TEST(t_table_read_write_seek_linear(), "read-write on a table without index (SHA1)");
+	TEST(t_table_read_write_seek_linear_sha256(), "read-write on a table without index (SHA256)");
+	TEST(t_table_read_write_sequential(), "sequential read-write on a table");
+	TEST(t_table_refs_for_no_index(), "refs-only table with no index");
+	TEST(t_table_refs_for_obj_index(), "refs-only table with index");
+	TEST(t_table_write_small_table(), "write_table works");
+	TEST(t_write_empty_key(), "write on refs with empty keys");
+	TEST(t_write_empty_table(), "read-write on empty tables");
+	TEST(t_write_key_order(), "refs must be written in increasing order");
+	TEST(t_write_multi_level_index(), "table with multi-level index");
+	TEST(t_write_multiple_indices(), "table with indices for multiple block types");
+	TEST(t_write_object_id_length(), "prefix compression on writing refs");
+	TEST(t_write_object_id_min_length(), "prefix compression on writing refs");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop

[Chandra Pratap]

free_names() as defined by reftable/basics.{c,h} frees a NULL
terminated array of malloced strings along with the array itself.
Use this function instead of a for loop to free such an array.

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-readwrite.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 235e3d94c7..e90f2bf9de 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -413,7 +413,6 @@ static void t_table_read_api(void)
 	struct reftable_reader rd = { 0 };
 	struct reftable_block_source source = { 0 };
 	int err;
-	int i;
 	struct reftable_log_record log = { 0 };
 	struct reftable_iterator it = { 0 };
 
@@ -432,10 +431,8 @@ static void t_table_read_api(void)
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+	free_names(names);
 	reftable_iterator_destroy(&it);
-	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
@@ -498,9 +495,7 @@ static void t_table_read_write_seek(int index, int hash_id)
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
-	reftable_free(names);
+	free_names(names);
 	reader_close(&rd);
 }
 
-- 
2.45.GIT

[PATCH v2 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops

[Chandra Pratap]

Using a for loop with an empty conditional statement is more concise
and easier to read than an infinite 'while' loop in instances
where we need a loop variable. Hence, replace such instances of a
'while' loop with the equivalent 'for' loop.

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-readwrite.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index e90f2bf9de..7daf28ec6d 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -268,15 +268,13 @@ static void t_log_write_read(void)
 	err = reftable_iterator_seek_log(&it, "");
 	check(!err);
 
-	i = 0;
-	while (1) {
+	for (i = 0; ; i++) {
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
 		check(!err);
 		check_str(names[i], log.refname);
 		check_int(i, ==, log.update_index);
-		i++;
 		reftable_log_record_release(&log);
 	}
 
@@ -374,7 +372,7 @@ static void t_table_read_write_sequential(void)
 	err = reftable_iterator_seek_ref(&it, "");
 	check(!err);
 
-	while (1) {
+	for (j = 0; ; j++) {
 		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
 		check_int(r, >=, 0);
@@ -382,8 +380,6 @@ static void t_table_read_write_sequential(void)
 			break;
 		check_str(names[j], ref.refname);
 		check_int(update_index, ==, ref.update_index);
-
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, N);
@@ -589,15 +585,13 @@ static void t_table_refs_for(int indexed)
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
 	check(!err);
 
-	j = 0;
-	while (1) {
+	for (j = 0; ; j++) {
 		int err = reftable_iterator_next_ref(&it, &ref);
 		check_int(err, >=, 0);
 		if (err > 0)
 			break;
 		check_int(j, <, want_names_len);
 		check_str(ref.refname, want_names[j]);
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, want_names_len);
-- 
2.45.GIT

[PATCH v2 4/4] t-reftable-readwrite: add test for known error

[Chandra Pratap]

When using reftable_writer_add_ref() to add a ref record to a
reftable writer, The update_index of the ref record must be within
the limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR
is returned. This scenario is currently left untested. Add a test
case 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-readwrite.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 7daf28ec6d..a5462441d3 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -773,6 +773,11 @@ static void t_write_key_order(void)
 	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
 	check_int(err, ==, REFTABLE_API_ERROR);
+
+	refs[0].update_index = 2;
+	err = reftable_writer_add_ref(w, &refs[0]);
+	check_int(err, ==, REFTABLE_API_ERROR);
+
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
-- 
2.45.GIT

Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I can see two options:
>
>   1. Refactor these interfaces such that they take a file descriptor as
>      input that they are writing to. This would allow us to exercise
>      that the output is correct.
>
>   2. Rip out this function. I don't think this functionality should be
>      part of the library in the first place, and it really only exists
>      because of "reftable/dump.c".
>
> I think the latter is the better option. The functionality exists to
> drive `cmd__dump_reftable()` in our reftable test helper. We should
> likely make the whole implementation of this an internal implementation
> detail and not expose it.

Thanks for a review.  Are there anything other than removing this
step that this series needs?

Re: [PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> reftable/readwrite_test.c exercises the functions defined in
> reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> reftable/readwrite_test.c to the unit testing framework. Migration
> involves refactoring the tests to use the unit testing framework
> instead of reftable's test framework and renaming the tests to
> align with unit-tests' naming conventions.
>
> Since some tests in reftable/readwrite_test.c use the functions
> set_test_hash(), noop_flush() and strbuf_add_void() defined in
> reftable/test_framework.{c,h} but these files are not #included
> in the ported unit test, copy these functions in the new test file.
>
> While at it, ensure structs are 0-initialized with '= { 0 }'
> instead of '= { NULL }'.

OK.

> -		EXPECT(buf->buf[off] == 'r');
> +		if (!off)
> +			off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
> +		check(buf->buf[off] == 'r');

Why not "check_char(buf->buf[off], ==, 'r')"?

>  	}
>  
> -	EXPECT(stats->log_stats.blocks > 0);
> +	check(stats->log_stats.blocks > 0);

Why not "check_int(stats->log_stats.blocks, >, 0)", which you used
in the t_log_write_read() function?

While reading this step, I looked for use of check() that is not
rewriting EXPECT_ERR(x) to check(!x) as suspicious.  The above two
(and a !memcmp() that is OK) were the only three such uses of
check(), I think.

Thanks.

Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> free_names() as defined by reftable/basics.{c,h} frees a NULL
> terminated array of malloced strings along with the array itself.
> Use this function instead of a for loop to free such an array.

Going back to [1/4], the headers included in this test looked like this:

    -#include "system.h"
    -
    -#include "basics.h"
    -#include "block.h"
    -#include "blocksource.h"
    -#include "reader.h"
    -#include "record.h"
    -#include "test_framework.h"
    -#include "reftable-tests.h"
    -#include "reftable-writer.h"
    +#include "test-lib.h"
    +#include "reftable/reader.h"
    +#include "reftable/blocksource.h"
    +#include "reftable/reftable-error.h"
    +#include "reftable/reftable-writer.h"

I found this part a bit curious, perhaps because I was not involved
in either reftable/ or unit-tests/ development.  So I may be asking
a stupid question, but is it intended that some headers like
"block.h" and "record.h" are no longer included?

It is understandable that inclusion of "test-lib.h" is new (and
needs to be there to work as part of t/unit-tests/), and the leading
directory name "reftable/" added to header files are also justified,
of course.  But if you depend on "basics.h" and do not include it,
that does not sound like the most hygenic thing to do, at least to
me.

The code changes themselves look good; I can see that the
implementation of free_names() in reftable/basics.c safely replaces
these loops.  There is a slight behaviour difference that names[]
that was fed to reftable_iterator_seek_ref() earlier goes away
before the iterator is destroyed, but _seek_ref() does not retain
the names[0] argument in the iterator object, so that is OK.

Thanks.

Re: [PATCH v2 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> Using a for loop with an empty conditional statement is more concise
> and easier to read than an infinite 'while' loop in instances
> where we need a loop variable. Hence, replace such instances of a
> 'while' loop with the equivalent 'for' loop.

Quite honestly, the above is counter-productive if pushed as a
general guideline, because the goodness of it depends what happens
in the third part of the for () control (i.e., what should happen at
the end of each iteration and if it wants to be bypassed in the
conditional inside the loop).

In this particular case, it probably is OK, but still is a
subjective, borderline Meh, to me.

I see no violation of correctness in the rewrite, though ;-)

Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop

[Chandra Pratap]

On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > free_names() as defined by reftable/basics.{c,h} frees a NULL
> > terminated array of malloced strings along with the array itself.
> > Use this function instead of a for loop to free such an array.
>
> Going back to [1/4], the headers included in this test looked like this:
>
>     -#include "system.h"
>     -
>     -#include "basics.h"
>     -#include "block.h"
>     -#include "blocksource.h"
>     -#include "reader.h"
>     -#include "record.h"
>     -#include "test_framework.h"
>     -#include "reftable-tests.h"
>     -#include "reftable-writer.h"
>     +#include "test-lib.h"
>     +#include "reftable/reader.h"
>     +#include "reftable/blocksource.h"
>     +#include "reftable/reftable-error.h"
>     +#include "reftable/reftable-writer.h"
>
> I found this part a bit curious, perhaps because I was not involved
> in either reftable/ or unit-tests/ development.  So I may be asking
> a stupid question, but is it intended that some headers like
> "block.h" and "record.h" are no longer included?
>
> It is understandable that inclusion of "test-lib.h" is new (and
> needs to be there to work as part of t/unit-tests/), and the leading
> directory name "reftable/" added to header files are also justified,
> of course.  But if you depend on "basics.h" and do not include it,
> that does not sound like the most hygenic thing to do, at least to
> me.

I think 'basics.{c,h}' in reftable/ is equivalent to 'stdio.h' in a generic
C program, it holds fundamental functionalities to be used by other
reftable structures and hence, is always (implicitly or explicitly)
#included in almost all of the files in reftable/.

This test is supposed to focus on reftable's read-write functionalities
so it makes sense to explicitly #include only those headers that
are directly responsible for those functionalities, namely 'reader.h',
'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in
there as well because some tests need to explicitly mention the
various error codes and it doesn't make sense to rely on it being
#included by others.

> The code changes themselves look good; I can see that the
> implementation of free_names() in reftable/basics.c safely replaces
> these loops.  There is a slight behaviour difference that names[]
> that was fed to reftable_iterator_seek_ref() earlier goes away
> before the iterator is destroyed, but _seek_ref() does not retain
> the names[0] argument in the iterator object, so that is OK.
>
> Thanks.

Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > free_names() as defined by reftable/basics.{c,h} frees a NULL
>> > terminated array of malloced strings along with the array itself.
>> > Use this function instead of a for loop to free such an array.
>> ...
> This test is supposed to focus on reftable's read-write functionalities
> so it makes sense to explicitly #include only those headers that
> are directly responsible for those functionalities, namely 'reader.h',
> 'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in
> there as well because some tests need to explicitly mention the
> various error codes and it doesn't make sense to rely on it being
> #included by others.

I think we are on the same page.  The code explicitly exercises
free_names() after this step, and that is exactly why I found it odd
to rely on basics.h happen to be included by some other header
file(s) we explicitly include.

Thanks.

Re: [PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

On Fri, 9 Aug 2024 at 23:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/readwrite_test.c exercises the functions defined in
> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> > reftable/readwrite_test.c to the unit testing framework. Migration
> > involves refactoring the tests to use the unit testing framework
> > instead of reftable's test framework and renaming the tests to
> > align with unit-tests' naming conventions.
> >
> > Since some tests in reftable/readwrite_test.c use the functions
> > set_test_hash(), noop_flush() and strbuf_add_void() defined in
> > reftable/test_framework.{c,h} but these files are not #included
> > in the ported unit test, copy these functions in the new test file.
> >
> > While at it, ensure structs are 0-initialized with '= { 0 }'
> > instead of '= { NULL }'.
>
> OK.
>
> > -             EXPECT(buf->buf[off] == 'r');
> > +             if (!off)
> > +                     off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
> > +             check(buf->buf[off] == 'r');
>
> Why not "check_char(buf->buf[off], ==, 'r')"?

I wrote this series quite some time ago when this functionality
was not yet introduced to the unit testing framework. I'll commit
this change in the next reroll.

> >       }
> >
> > -     EXPECT(stats->log_stats.blocks > 0);
> > +     check(stats->log_stats.blocks > 0);
>
> Why not "check_int(stats->log_stats.blocks, >, 0)", which you used
> in the t_log_write_read() function?

Looks like a case of too-mechanical-a-translation to me. I'll fix this
in the next version.

> While reading this step, I looked for use of check() that is not
> rewriting EXPECT_ERR(x) to check(!x) as suspicious.  The above two
> (and a !memcmp() that is OK) were the only three such uses of
> check(), I think.

I went through this series again and I agree on not encountering
any other subpar translations of EXPECT() to its counterparts in
the unit testing framework. I'll reroll the series with only these
changes until someone else finds any other corrections.

[GSoC][PATCH v3 0/4] t: port reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/readwrite_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:
- Order the header files alphabetically in
  t/unit-tests/t-reftable-readwrite.c in patch 1.
- use check_char() and check_int() instead of check() at two
  instances in patch 1.
- #include 'reftable/basics.h' in patch 2 when introducing
  free_names() in the test file.

CI/PR: https://github.com/gitgitgadget/git/pull/1770

Chandra Pratap(4):
t: move reftable/readwrite_test.c to the unit testing framework
t-reftable-readwrite: use free_names() instead of a for loop
t-reftable-readwrite: use 'for' in place of infinite 'while' loops
t-reftable-readwrite: add test for known error

Makefile                                                         |   2 +-
reftable/reftable-tests.h                                        |   1 -
t/helper/test-reftable.c                                         |   1 -
reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 441 +++++++++++++++++++++++-----------------
4 files changed, 219 insertions(+), 226 deletions(-)

Range-diff against v2:
1:  0ebe76c331 ! 1:  03b946434e t: move reftable/readwrite_test.c to the unit testing framework
    @@ t/unit-tests/t-reftable-readwrite.c: license that can be found in the LICENSE fi
     -#include "reftable-tests.h"
     -#include "reftable-writer.h"
     +#include "test-lib.h"
    -+#include "reftable/reader.h"
     +#include "reftable/blocksource.h"
    ++#include "reftable/reader.h"
     +#include "reftable/reftable-error.h"
     +#include "reftable/reftable-writer.h"

    @@ t/unit-tests/t-reftable-readwrite.c: static void write_table(char ***names, stru
     -		EXPECT(buf->buf[off] == 'r');
     +		if (!off)
     +			off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
    -+		check(buf->buf[off] == 'r');
    ++		check_char(buf->buf[off], ==, 'r');
      	}

     -	EXPECT(stats->log_stats.blocks > 0);
    -+	check(stats->log_stats.blocks > 0);
    ++	check_int(stats->log_stats.blocks, >, 0);
      	reftable_writer_free(w);
      }

2:  a148702451 ! 2:  e23a515736 t-reftable-readwrite: use free_names() instead of a for loop
    @@ Commit message
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

      ## t/unit-tests/t-reftable-readwrite.c ##
    +@@ t/unit-tests/t-reftable-readwrite.c: license that can be found in the LICENSE file or at
    + */
    +
    + #include "test-lib.h"
    ++#include "reftable/basics.h"
    + #include "reftable/blocksource.h"
    + #include "reftable/reader.h"
    + #include "reftable/reftable-error.h"
     @@ t/unit-tests/t-reftable-readwrite.c: static void t_table_read_api(void)
      	struct reftable_reader rd = { 0 };
      	struct reftable_block_source source = { 0 };
3:  ee15af6631 = 3:  9194a4055a t-reftable-readwrite: use 'for' in place of infinite 'while' loops
4:  3f571c09e2 = 4:  d34b01fad8 t-reftable-readwrite: add test for known error

[PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

reftable/readwrite_test.c exercises the functions defined in
reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
reftable/readwrite_test.c to the unit testing framework. Migration
involves refactoring the tests to use the unit testing framework
instead of reftable's test framework and renaming the tests to
align with unit-tests' naming conventions.

Since some tests in reftable/readwrite_test.c use the functions
set_test_hash(), noop_flush() and strbuf_add_void() defined in
reftable/test_framework.{c,h} but these files are not #included
in the ported unit test, copy these functions in the new test file.

While at it, ensure structs are 0-initialized with '= { 0 }'
instead of '= { NULL }'.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 reftable/reftable-tests.h                     |   1 -
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------
 4 files changed, 210 insertions(+), 212 deletions(-)
 rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)

diff --git a/Makefile b/Makefile
index 3863e60b66..76e4d1c1ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-merged
+UNIT_TEST_PROGRAMS += t-reftable-readwrite
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
 REFTABLE_TEST_OBJS += reftable/tree_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..7d955393d2 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);
 int block_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
-int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9d378427da..d371e9f9dd 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	block_test_main(argc, argv);
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
-	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c
similarity index 70%
rename from reftable/readwrite_test.c
rename to t/unit-tests/t-reftable-readwrite.c
index f411abfe9c..d0eb85fc38 100644
--- a/reftable/readwrite_test.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-
-#include "basics.h"
-#include "block.h"
-#include "blocksource.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-#include "reftable-writer.h"
+#include "test-lib.h"
+#include "reftable/blocksource.h"
+#include "reftable/reader.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-writer.h"
 
 static const int update_index = 5;
 
-static void test_buffer(void)
+static void set_test_hash(uint8_t *p, int i)
+{
+	memset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));
+}
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
+
+static int noop_flush(void *arg)
+{
+	return 0;
+}
+
+static void t_buffer(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_block out = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_block out = { 0 };
 	int n;
 	uint8_t in[] = "hello";
 	strbuf_add(&buf, in, sizeof(in));
 	block_source_from_strbuf(&source, &buf);
-	EXPECT(block_source_size(&source) == 6);
+	check_int(block_source_size(&source), ==, 6);
 	n = block_source_read_block(&source, &out, 0, sizeof(in));
-	EXPECT(n == sizeof(in));
-	EXPECT(!memcmp(in, out.data, n));
+	check_int(n, ==, sizeof(in));
+	check(!memcmp(in, out.data, n));
 	reftable_block_done(&out);
 
 	n = block_source_read_block(&source, &out, 1, 2);
-	EXPECT(n == 2);
-	EXPECT(!memcmp(out.data, "el", 2));
+	check_int(n, ==, 2);
+	check(!memcmp(out.data, "el", 2));
 
 	reftable_block_done(&out);
 	block_source_close(&source);
@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 	};
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0, n;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	const struct reftable_stats *stats = NULL;
 
 	REFTABLE_CALLOC_ARRAY(*names, N + 1);
@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		(*names)[i] = xstrdup(name);
 
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	for (i = 0; i < N; i++) {
@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		log.value.update.message = (char *) "message";
 
 		n = reftable_writer_add_log(w, &log);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
 	for (i = 0; i < stats->ref_stats.blocks; i++) {
 		int off = i * opts.block_size;
-		if (off == 0) {
-			off = header_size(
-				(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
-		}
-		EXPECT(buf->buf[off] == 'r');
+		if (!off)
+			off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
+		check_char(buf->buf[off], ==, 'r');
 	}
 
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 }
 
-static void test_log_buffer_size(void)
+static void t_log_buffer_size(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_write_options opts = {
@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)
 	}
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_overflow(void)
+static void t_log_overflow(void)
 {
 	struct strbuf buf = STRBUF_INIT;
 	char msg[256] = { 0 };
@@ -177,12 +186,12 @@ static void test_log_overflow(void)
 	memset(msg, 'x', sizeof(msg) - 1);
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
-	EXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);
+	check_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_log_write_read(void)
+static void t_log_write_read(void)
 {
 	int N = 2;
 	char **names = reftable_calloc(N + 1, sizeof(*names));
@@ -190,13 +199,13 @@ static void test_log_write_read(void)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
-	struct reftable_log_record log = { NULL };
+	struct reftable_log_record log = { 0 };
 	int n;
-	struct reftable_iterator it = { NULL };
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
@@ -204,17 +213,17 @@ static void test_log_write_read(void)
 	reftable_writer_set_limits(w, 0, N);
 	for (i = 0; i < N; i++) {
 		char name[256];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		snprintf(name, sizeof(name), "b%02d%0*d", i, 130, 7);
 		names[i] = xstrdup(name);
 		ref.refname = name;
 		ref.update_index = i;
 
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	for (i = 0; i < N; i++) {
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 
 		log.refname = names[i];
 		log.update_index = i;
@@ -223,33 +232,33 @@ static void test_log_write_read(void)
 		set_test_hash(log.value.update.new_hash, i + 1);
 
 		err = reftable_writer_add_log(w, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_ref(&it, names[N - 1]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
+	check(!err);
 
 	/* end of iteration. */
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT(0 < err);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_ref_record_release(&ref);
@@ -257,23 +266,21 @@ static void test_log_write_read(void)
 	reftable_reader_init_log_iterator(&rd, &it);
 
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	i = 0;
 	while (1) {
 		int err = reftable_iterator_next_log(&it, &log);
-		if (err > 0) {
+		if (err > 0)
 			break;
-		}
-
-		EXPECT_ERR(err);
-		EXPECT_STREQ(names[i], log.refname);
-		EXPECT(i == log.update_index);
+		check(!err);
+		check_str(names[i], log.refname);
+		check_int(i, ==, log.update_index);
 		i++;
 		reftable_log_record_release(&log);
 	}
 
-	EXPECT(i == N);
+	check_int(i, ==, N);
 	reftable_iterator_destroy(&it);
 
 	/* cleanup. */
@@ -282,7 +289,7 @@ static void test_log_write_read(void)
 	reader_close(&rd);
 }
 
-static void test_log_zlib_corruption(void)
+static void t_log_zlib_corruption(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 256,
@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_log(w, &log);
-	EXPECT_ERR(err);
+	check(!err);
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	stats = reftable_writer_stats(w);
-	EXPECT(stats->log_stats.blocks > 0);
+	check_int(stats->log_stats.blocks, >, 0);
 	reftable_writer_free(w);
 	w = NULL;
 
@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_log_iterator(&rd, &it);
 	err = reftable_iterator_seek_log(&it, "refname");
-	EXPECT(err == REFTABLE_ZLIB_ERROR);
+	check_int(err, ==, REFTABLE_ZLIB_ERROR);
 
 	reftable_iterator_destroy(&it);
 
@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)
 	reader_close(&rd);
 }
 
-static void test_table_read_write_sequential(void)
+static void t_table_read_write_sequential(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_iterator it = { NULL };
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_iterator it = { 0 };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err = 0;
 	int j = 0;
 
@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	while (1) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(r >= 0);
-		if (r > 0) {
+		check_int(r, >=, 0);
+		if (r > 0)
 			break;
-		}
-		EXPECT(0 == strcmp(names[j], ref.refname));
-		EXPECT(update_index == ref.update_index);
+		check_str(names[j], ref.refname);
+		check_int(update_index, ==, ref.update_index);
 
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == N);
+	check_int(j, ==, N);
 	reftable_iterator_destroy(&it);
 	strbuf_release(&buf);
 	free_names(names);
@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)
 	reader_close(&rd);
 }
 
-static void test_table_write_small_table(void)
+static void t_table_write_small_table(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 1;
 	write_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);
-	EXPECT(buf.len < 200);
+	check_int(buf.len, <, 200);
 	strbuf_release(&buf);
 	free_names(names);
 }
 
-static void test_table_read_api(void)
+static void t_table_read_api(void)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i;
-	struct reftable_log_record log = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_log_record log = { 0 };
+	struct reftable_iterator it = { 0 };
 
 	write_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, names[0]);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_log(&it, &log);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_iterator_destroy(&it);
 	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
 
-static void test_table_read_write_seek(int index, int hash_id)
+static void t_table_read_write_seek(int index, int hash_id)
 {
 	char **names;
 	struct strbuf buf = STRBUF_INIT;
 	int N = 50;
-	struct reftable_reader rd = { NULL };
-	struct reftable_block_source source = { NULL };
+	struct reftable_reader rd = { 0 };
+	struct reftable_block_source source = { 0 };
 	int err;
 	int i = 0;
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	struct strbuf pastLast = STRBUF_INIT;
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 
 	write_table(&names, &buf, N, 256, hash_id);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	EXPECT(hash_id == reftable_reader_hash_id(&rd));
+	check(!err);
+	check_int(hash_id, ==, reftable_reader_hash_id(&rd));
 
-	if (!index) {
+	if (!index)
 		rd.ref_offsets.index_offset = 0;
-	} else {
-		EXPECT(rd.ref_offsets.index_offset > 0);
-	}
+	else
+		check_int(rd.ref_offsets.index_offset, >, 0);
 
 	for (i = 1; i < N; i++) {
 		reftable_reader_init_ref_iterator(&rd, &it);
 		err = reftable_iterator_seek_ref(&it, names[i]);
-		EXPECT_ERR(err);
+		check(!err);
 		err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT_ERR(err);
-		EXPECT(0 == strcmp(names[i], ref.refname));
-		EXPECT(REFTABLE_REF_VAL1 == ref.value_type);
-		EXPECT(i == ref.value.val1[0]);
+		check(!err);
+		check_str(names[i], ref.refname);
+		check_int(REFTABLE_REF_VAL1, ==, ref.value_type);
+		check_int(i, ==, ref.value.val1[0]);
 
 		reftable_ref_record_release(&ref);
 		reftable_iterator_destroy(&it);
@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, pastLast.buf);
 	if (err == 0) {
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	} else {
-		EXPECT(err > 0);
+		check_int(err, >, 0);
 	}
 
 	strbuf_release(&pastLast);
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 	reftable_free(names);
 	reader_close(&rd);
 }
 
-static void test_table_read_write_seek_linear(void)
+static void t_table_read_write_seek_linear(void)
 {
-	test_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_linear_sha256(void)
+static void t_table_read_write_seek_linear_sha256(void)
 {
-	test_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
+	t_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);
 }
 
-static void test_table_read_write_seek_index(void)
+static void t_table_read_write_seek_index(void)
 {
-	test_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
+	t_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_refs_for(int indexed)
+static void t_table_refs_for(int indexed)
 {
 	int N = 50;
 	char **want_names = reftable_calloc(N + 1, sizeof(*want_names));
@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)
 	struct reftable_write_options opts = {
 		.block_size = 256,
 	};
-	struct reftable_ref_record ref = { NULL };
+	struct reftable_ref_record ref = { 0 };
 	int i = 0;
 	int n;
 	int err;
 	struct reftable_reader rd;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
 
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int j;
 
 	set_test_hash(want_hash, 4);
@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)
 		uint8_t hash[GIT_SHA1_RAWSZ];
 		char fill[51] = { 0 };
 		char name[100];
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 
 		memset(hash, i, sizeof(hash));
 		memset(fill, 'x', 50);
@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)
 		 */
 		/* blocks. */
 		n = reftable_writer_add_ref(w, &ref);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		if (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||
-		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {
+		    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))
 			want_names[want_names_len++] = xstrdup(name);
-		}
 	}
 
 	n = reftable_writer_close(w);
-	EXPECT(n == 0);
+	check_int(n, ==, 0);
 
 	reftable_writer_free(w);
 	w = NULL;
@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)
 	block_source_from_strbuf(&source, &buf);
 
 	err = init_reader(&rd, &source, "file.ref");
-	EXPECT_ERR(err);
-	if (!indexed) {
+	check(!err);
+	if (!indexed)
 		rd.obj_offsets.is_present = 0;
-	}
 
 	reftable_reader_init_ref_iterator(&rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_iterator_destroy(&it);
 
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
-	EXPECT_ERR(err);
+	check(!err);
 
 	j = 0;
 	while (1) {
 		int err = reftable_iterator_next_ref(&it, &ref);
-		EXPECT(err >= 0);
-		if (err > 0) {
+		check_int(err, >=, 0);
+		if (err > 0)
 			break;
-		}
-
-		EXPECT(j < want_names_len);
-		EXPECT(0 == strcmp(ref.refname, want_names[j]));
+		check_int(j, <, want_names_len);
+		check_str(ref.refname, want_names[j]);
 		j++;
 		reftable_ref_record_release(&ref);
 	}
-	EXPECT(j == want_names_len);
+	check_int(j, ==, want_names_len);
 
 	strbuf_release(&buf);
 	free_names(want_names);
@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)
 	reader_close(&rd);
 }
 
-static void test_table_refs_for_no_index(void)
+static void t_table_refs_for_no_index(void)
 {
-	test_table_refs_for(0);
+	t_table_refs_for(0);
 }
 
-static void test_table_refs_for_obj_index(void)
+static void t_table_refs_for_obj_index(void)
 {
-	test_table_refs_for(1);
+	t_table_refs_for(1);
 }
 
-static void test_write_empty_table(void)
+static void t_write_empty_table(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
 		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_reader *rd = NULL;
-	struct reftable_ref_record rec = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record rec = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 
-	EXPECT(buf.len == header_size(1) + footer_size(1));
+	check_int(buf.len, ==, header_size(1) + footer_size(1));
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_init_ref_iterator(rd, &it);
 	err = reftable_iterator_seek_ref(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &rec);
-	EXPECT(err > 0);
+	check_int(err, >, 0);
 
 	reftable_iterator_destroy(&it);
 	reftable_reader_free(rd);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_min_length(void)
+static void t_write_object_id_min_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)
 		snprintf(name, sizeof(name), "ref%05d", i);
 		ref.refname = name;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 2);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 2);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_object_id_length(void)
+static void t_write_object_id_length(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 75,
@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)
 		ref.refname = name;
 		ref.value.val1[15] = i;
 		err = reftable_writer_add_ref(w, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
-	EXPECT(reftable_writer_stats(w)->object_id_len == 16);
+	check(!err);
+	check_int(reftable_writer_stats(w)->object_id_len, ==, 16);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_empty_key(void)
+static void t_write_empty_key(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -747,15 +746,15 @@ static void test_write_empty_key(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &ref);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 
 	err = reftable_writer_close(w);
-	EXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);
+	check_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_key_order(void)
+static void t_write_key_order(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -782,15 +781,15 @@ static void test_write_key_order(void)
 
 	reftable_writer_set_limits(w, 1, 1);
 	err = reftable_writer_add_ref(w, &refs[0]);
-	EXPECT_ERR(err);
+	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
-	EXPECT(err == REFTABLE_API_ERROR);
+	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
 }
 
-static void test_write_multiple_indices(void)
+static void t_write_multiple_indices(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	for (i = 0; i < 100; i++) {
@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)
 		log.refname = buf.buf,
 
 		err = reftable_writer_add_log(writer, &log);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	reftable_writer_close(writer);
@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)
 	 * for each of the block types.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.index_offset > 0);
-	EXPECT(stats->obj_stats.index_offset > 0);
-	EXPECT(stats->log_stats.index_offset > 0);
+	check_int(stats->ref_stats.index_offset, >, 0);
+	check_int(stats->obj_stats.index_offset, >, 0);
+	check_int(stats->log_stats.index_offset, >, 0);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the log uses the log index now. In case there is any
@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)
 	 */
 	reftable_reader_init_log_iterator(reader, &it);
 	err = reftable_iterator_seek_log(&it, "");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)
 	strbuf_release(&buf);
 }
 
-static void test_write_multi_level_index(void)
+static void t_write_multi_level_index(void)
 {
 	struct reftable_write_options opts = {
 		.block_size = 100,
@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)
 		ref.refname = buf.buf,
 
 		err = reftable_writer_add_ref(writer, &ref);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 	reftable_writer_close(writer);
 
@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)
 	 * multi-level index.
 	 */
 	stats = reftable_writer_stats(writer);
-	EXPECT(stats->ref_stats.max_index_level == 2);
+	check_int(stats->ref_stats.max_index_level, ==, 2);
 
 	block_source_from_strbuf(&source, &writer_buf);
 	err = reftable_new_reader(&reader, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	/*
 	 * Seeking the last ref should work as expected.
 	 */
 	reftable_reader_init_ref_iterator(reader, &it);
 	err = reftable_iterator_seek_ref(&it, "refs/heads/199");
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_iterator_destroy(&it);
 	reftable_writer_free(writer);
@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)
 	strbuf_release(&buf);
 }
 
-static void test_corrupt_table_empty(void)
+static void t_corrupt_table_empty(void)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 }
 
-static void test_corrupt_table(void)
+static void t_corrupt_table(void)
 {
 	uint8_t zeros[1024] = { 0 };
 	struct strbuf buf = STRBUF_INIT;
-	struct reftable_block_source source = { NULL };
-	struct reftable_reader rd = { NULL };
+	struct reftable_block_source source = { 0 };
+	struct reftable_reader rd = { 0 };
 	int err;
 	strbuf_add(&buf, zeros, sizeof(zeros));
 
 	block_source_from_strbuf(&source, &buf);
 	err = init_reader(&rd, &source, "file.log");
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	strbuf_release(&buf);
 }
 
-int readwrite_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_log_zlib_corruption);
-	RUN_TEST(test_corrupt_table);
-	RUN_TEST(test_corrupt_table_empty);
-	RUN_TEST(test_log_write_read);
-	RUN_TEST(test_write_key_order);
-	RUN_TEST(test_table_read_write_seek_linear_sha256);
-	RUN_TEST(test_log_buffer_size);
-	RUN_TEST(test_table_write_small_table);
-	RUN_TEST(test_buffer);
-	RUN_TEST(test_table_read_api);
-	RUN_TEST(test_table_read_write_sequential);
-	RUN_TEST(test_table_read_write_seek_linear);
-	RUN_TEST(test_table_read_write_seek_index);
-	RUN_TEST(test_table_refs_for_no_index);
-	RUN_TEST(test_table_refs_for_obj_index);
-	RUN_TEST(test_write_empty_key);
-	RUN_TEST(test_write_empty_table);
-	RUN_TEST(test_log_overflow);
-	RUN_TEST(test_write_object_id_length);
-	RUN_TEST(test_write_object_id_min_length);
-	RUN_TEST(test_write_multiple_indices);
-	RUN_TEST(test_write_multi_level_index);
-	return 0;
+	TEST(t_buffer(), "strbuf works as blocksource");
+	TEST(t_corrupt_table(), "read-write on corrupted table");
+	TEST(t_corrupt_table_empty(), "read-write on an empty table");
+	TEST(t_log_buffer_size(), "buffer extension for log compression");
+	TEST(t_log_overflow(), "log overflow returns expected error");
+	TEST(t_log_write_read(), "read-write on log records");
+	TEST(t_log_zlib_corruption(), "reading corrupted log record returns expected error");
+	TEST(t_table_read_api(), "read on a table");
+	TEST(t_table_read_write_seek_index(), "read-write on a table with index");
+	TEST(t_table_read_write_seek_linear(), "read-write on a table without index (SHA1)");
+	TEST(t_table_read_write_seek_linear_sha256(), "read-write on a table without index (SHA256)");
+	TEST(t_table_read_write_sequential(), "sequential read-write on a table");
+	TEST(t_table_refs_for_no_index(), "refs-only table with no index");
+	TEST(t_table_refs_for_obj_index(), "refs-only table with index");
+	TEST(t_table_write_small_table(), "write_table works");
+	TEST(t_write_empty_key(), "write on refs with empty keys");
+	TEST(t_write_empty_table(), "read-write on empty tables");
+	TEST(t_write_key_order(), "refs must be written in increasing order");
+	TEST(t_write_multi_level_index(), "table with multi-level index");
+	TEST(t_write_multiple_indices(), "table with indices for multiple block types");
+	TEST(t_write_object_id_length(), "prefix compression on writing refs");
+	TEST(t_write_object_id_min_length(), "prefix compression on writing refs");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v3 2/4] t-reftable-readwrite: use free_names() instead of a for loop

[Chandra Pratap]

free_names() as defined by reftable/basics.{c,h} frees a NULL
terminated array of malloced strings along with the array itself.
Use this function instead of a for loop to free such an array.

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-readwrite.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index d0eb85fc38..8e546b0dd6 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at
 */
 
 #include "test-lib.h"
+#include "reftable/basics.h"
 #include "reftable/blocksource.h"
 #include "reftable/reader.h"
 #include "reftable/reftable-error.h"
@@ -413,7 +414,6 @@ static void t_table_read_api(void)
 	struct reftable_reader rd = { 0 };
 	struct reftable_block_source source = { 0 };
 	int err;
-	int i;
 	struct reftable_log_record log = { 0 };
 	struct reftable_iterator it = { 0 };
 
@@ -432,10 +432,8 @@ static void t_table_read_api(void)
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+	free_names(names);
 	reftable_iterator_destroy(&it);
-	reftable_free(names);
 	reader_close(&rd);
 	strbuf_release(&buf);
 }
@@ -498,9 +496,7 @@ static void t_table_read_write_seek(int index, int hash_id)
 	reftable_iterator_destroy(&it);
 
 	strbuf_release(&buf);
-	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
-	reftable_free(names);
+	free_names(names);
 	reader_close(&rd);
 }
 
-- 
2.45.GIT

[PATCH v3 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops

[Chandra Pratap]

Using a for loop with an empty conditional statement is more concise
and easier to read than an infinite 'while' loop in instances
where we need a loop variable. Hence, replace such instances of a
'while' loop with the equivalent 'for' loop.

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-readwrite.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 8e546b0dd6..9a05dde9d6 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -269,15 +269,13 @@ static void t_log_write_read(void)
 	err = reftable_iterator_seek_log(&it, "");
 	check(!err);
 
-	i = 0;
-	while (1) {
+	for (i = 0; ; i++) {
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
 		check(!err);
 		check_str(names[i], log.refname);
 		check_int(i, ==, log.update_index);
-		i++;
 		reftable_log_record_release(&log);
 	}
 
@@ -375,7 +373,7 @@ static void t_table_read_write_sequential(void)
 	err = reftable_iterator_seek_ref(&it, "");
 	check(!err);
 
-	while (1) {
+	for (j = 0; ; j++) {
 		struct reftable_ref_record ref = { 0 };
 		int r = reftable_iterator_next_ref(&it, &ref);
 		check_int(r, >=, 0);
@@ -383,8 +381,6 @@ static void t_table_read_write_sequential(void)
 			break;
 		check_str(names[j], ref.refname);
 		check_int(update_index, ==, ref.update_index);
-
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, N);
@@ -590,15 +586,13 @@ static void t_table_refs_for(int indexed)
 	err = reftable_reader_refs_for(&rd, &it, want_hash);
 	check(!err);
 
-	j = 0;
-	while (1) {
+	for (j = 0; ; j++) {
 		int err = reftable_iterator_next_ref(&it, &ref);
 		check_int(err, >=, 0);
 		if (err > 0)
 			break;
 		check_int(j, <, want_names_len);
 		check_str(ref.refname, want_names[j]);
-		j++;
 		reftable_ref_record_release(&ref);
 	}
 	check_int(j, ==, want_names_len);
-- 
2.45.GIT

[PATCH v3 4/4] t-reftable-readwrite: add test for known error

[Chandra Pratap]

When using reftable_writer_add_ref() to add a ref record to a
reftable writer, The update_index of the ref record must be within
the limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR
is returned. This scenario is currently left untested. Add a test
case 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-readwrite.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 9a05dde9d6..2ce56a0523 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -774,6 +774,11 @@ static void t_write_key_order(void)
 	check(!err);
 	err = reftable_writer_add_ref(w, &refs[1]);
 	check_int(err, ==, REFTABLE_API_ERROR);
+
+	refs[0].update_index = 2;
+	err = reftable_writer_add_ref(w, &refs[0]);
+	check_int(err, ==, REFTABLE_API_ERROR);
+
 	reftable_writer_close(w);
 	reftable_writer_free(w);
 	strbuf_release(&buf);
-- 
2.45.GIT

Re: [GSoC][PATCH v3 0/4] t: port reftable/readwrite_test.c to the unit testing framework

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> Changes in v3:
> - Order the header files alphabetically in
>   t/unit-tests/t-reftable-readwrite.c in patch 1.
> - use check_char() and check_int() instead of check() at two
>   instances in patch 1.
> - #include 'reftable/basics.h' in patch 2 when introducing
>   free_names() in the test file.
>
> CI/PR: https://github.com/gitgitgadget/git/pull/1770
>
> Chandra Pratap(4):
> t: move reftable/readwrite_test.c to the unit testing framework
> t-reftable-readwrite: use free_names() instead of a for loop
> t-reftable-readwrite: use 'for' in place of infinite 'while' loops
> t-reftable-readwrite: add test for known error

Will replace.

Let's see if we get any more comments and otherwise let me mark the
topic for 'next' soonish.

Thanks.

Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Josh Steadmon]

On 2024.08.13 20:04, Chandra Pratap wrote:
> reftable/readwrite_test.c exercises the functions defined in
> reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> reftable/readwrite_test.c to the unit testing framework. Migration
> involves refactoring the tests to use the unit testing framework
> instead of reftable's test framework and renaming the tests to
> align with unit-tests' naming conventions.
> 
> Since some tests in reftable/readwrite_test.c use the functions
> set_test_hash(), noop_flush() and strbuf_add_void() defined in
> reftable/test_framework.{c,h} but these files are not #included
> in the ported unit test, copy these functions in the new test file.

I'm assuming that eventually, reftable/test_framework (and all the rest
of reftable/libreftable_test.a) will be removed after all the tests are
converted to the unit test framework, is that correct? Will other tests
need these test_framework functions? If so, I'd rather not end up with
duplicates in each test file, even if these are small functions. Is
there a reason why we can't link the reftable/test_framework object (or
the whole reftable/libreftable_test.a library)?

Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Chandra Pratap]

On Wed, 14 Aug 2024 at 04:03, Josh Steadmon <steadmon@google.com> wrote:
>
> On 2024.08.13 20:04, Chandra Pratap wrote:
> > reftable/readwrite_test.c exercises the functions defined in
> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> > reftable/readwrite_test.c to the unit testing framework. Migration
> > involves refactoring the tests to use the unit testing framework
> > instead of reftable's test framework and renaming the tests to
> > align with unit-tests' naming conventions.
> >
> > Since some tests in reftable/readwrite_test.c use the functions
> > set_test_hash(), noop_flush() and strbuf_add_void() defined in
> > reftable/test_framework.{c,h} but these files are not #included
> > in the ported unit test, copy these functions in the new test file.
>
> I'm assuming that eventually, reftable/test_framework (and all the rest
> of reftable/libreftable_test.a) will be removed after all the tests are
> converted to the unit test framework, is that correct?

That hasn't been discussed yet but yes, that seems the most likely
fate for reftable/test_framework.{c,h}.

> Will other tests need these test_framework functions? If so, I'd rather
> not end up with duplicates in each test file, even if these are small
> functions. Is there a reason why we can't link the reftable/test_framework
> object (or the whole reftable/libreftable_test.a library)?

If I remember correctly, only stack and merged tests besides readwrite
utilize these functions.

We're not #including 'test_framwork.h' in the new test files because in
a way, the point of this GSoC project is to get rid of
reftable/test_framework.{c,h} so we don't need to carry an entirely
different testing framework for the reftable sub-project.

As far as duplicated functions are concerned, we can maybe move
them to unit-tests/test-lib.{c,h} or a new file in reftable/. I'll create
a patch for it sometimes later.

Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework

[Patrick Steinhardt]

On Tue, Aug 13, 2024 at 03:33:04PM -0700, Josh Steadmon wrote:
> On 2024.08.13 20:04, Chandra Pratap wrote:
> > reftable/readwrite_test.c exercises the functions defined in
> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> > reftable/readwrite_test.c to the unit testing framework. Migration
> > involves refactoring the tests to use the unit testing framework
> > instead of reftable's test framework and renaming the tests to
> > align with unit-tests' naming conventions.
> > 
> > Since some tests in reftable/readwrite_test.c use the functions
> > set_test_hash(), noop_flush() and strbuf_add_void() defined in
> > reftable/test_framework.{c,h} but these files are not #included
> > in the ported unit test, copy these functions in the new test file.
> 
> I'm assuming that eventually, reftable/test_framework (and all the rest
> of reftable/libreftable_test.a) will be removed after all the tests are
> converted to the unit test framework, is that correct? Will other tests
> need these test_framework functions? If so, I'd rather not end up with
> duplicates in each test file, even if these are small functions. Is
> there a reason why we can't link the reftable/test_framework object (or
> the whole reftable/libreftable_test.a library)?

The reason is likely that they use different infra, e.g. `EXPECT()` vs
`check()`. So instead of linking `libreftable_test.a`, I think it is
fine to duplicate the functionality in `t/unit-tests`. In not too
distant of a future we're going to get rid of everything in the reftable
tests anyway, including the `libreftable_test.a` library. So avoiding
the duplication doesn't make a ton of sense to me.

That being said, I think we should not duplicate functionality in
`t/unit-tests`. So if there is functionality used by multiple tests, we
should likely move it into a new `t/unit-tests/lib-reftable.c` file.

Patrick