Re: [PATCH v2 03/10] t/unit-tests: convert reftable block test to use clar
From: Patrick Steinhardt <hidden>
Date: 2025-05-02 09:57:50
On Tue, Apr 29, 2025 at 06:52:55PM +0100, Seyi Kuforiji wrote:
quoted hunk ↗ jump to hunk
diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c deleted file mode 100644 index 22040aeefa..0000000000 --- a/t/unit-tests/t-reftable-block.c +++ /dev/null
Hm, why is this recorded as a delete and creation? Weird, inspecting the diff locally properly shows it as a rename, which makes it a ton easier to review. It would be great if you could try to play around with the `--find-renames` option in the next iteration of this series and double check that these are shown as a rename.
quoted hunk ↗ jump to hunk
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c new file mode 100644 index 0000000000..af24901230 --- /dev/null +++ b/t/unit-tests/u-reftable-block.c@@ -0,0 +1,373 @@ +/* +Copyright 2020 Google LLC + +Use of this source code is governed by a BSD-style +license that can be found in the LICENSE file or at +https://developers.google.com/open-source/licenses/bsd +*/ + +#include "unit-test.h" +#include "reftable/block.h" +#include "reftable/blocksource.h" +#include "reftable/constants.h" +#include "reftable/reftable-error.h" +#include "strbuf.h" + +void test_reftable_block__index_read_write(void)
This doesn't got to do anything with indices but with refs, so I'd rename this to `__ref_read_write()`.
+{
+ const int header_off = 21; /* random */
+ struct reftable_record recs[30];
+ const size_t N = ARRAY_SIZE(recs);
+ const size_t block_size = 1024;
+ struct reftable_block block = { 0 };
+ struct block_writer bw = {
+ .last_key = REFTABLE_BUF_INIT,
+ };
+ struct reftable_record rec = {
+ .type = BLOCK_TYPE_REF,
+ };
+ size_t i = 0;
+ int ret;
+ struct block_reader br = { 0 };
+ struct block_iter it = BLOCK_ITER_INIT;
+ struct reftable_buf want = REFTABLE_BUF_INIT, buf = REFTABLE_BUF_INIT;
+
+ REFTABLE_CALLOC_ARRAY(block.data, block_size);
+ cl_assert(block.data != NULL);
+ block.len = block_size;
+ block_source_from_buf(&block.source ,&buf);
+ ret = block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,
+ header_off, hash_size(REFTABLE_HASH_SHA1));
+ cl_assert(ret == 0);Same comment here, asserts like this can be retained as `cl_assert(!ret)`.
+ rec.u.ref.refname = (char *) "";
+ rec.u.ref.value_type = REFTABLE_REF_DELETION;
+ ret = block_writer_add(&bw, &rec);
+ cl_assert_equal_i(ret, REFTABLE_API_ERROR);
+
+ for (i = 0; i < N; i++) {
+ rec.u.ref.refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
+ rec.u.ref.value_type = REFTABLE_REF_VAL1;
+ memset(rec.u.ref.value.val1, i, REFTABLE_HASH_SIZE_SHA1);
+
+ recs[i] = rec;
+ ret = block_writer_add(&bw, &rec);
+ rec.u.ref.refname = NULL;
+ rec.u.ref.value_type = REFTABLE_REF_DELETION;
+ cl_assert_equal_i(ret, 0);
+ }
+
+ ret = block_writer_finish(&bw);
+ cl_assert(ret > 0);It's a bit unfortunate that we have to use `cl_assert()` here, but that isn't the fault of this series. I do have a pull request pending upstream that introduces integer comparisons. Once we've updated to that version I'll go through our unit tests and adapt callsites accordingly. [snip]
+void test_reftable_block__ref_read_write(void)
This one here should be called `__index_read_write()`. I guess you confused the first and and this test name with one another. Patrick