On Tue, Apr 29, 2025 at 06:52:54PM +0100, Seyi Kuforiji wrote:
> Adapt reftable basics test file to clar by using clar assertions
> where necessary.Break up test edge case to improve modularity and
> clarity.
>
> Signed-off-by: Seyi Kuforiji <kuforiji98@xxxxxxxxx>
> ---
> Makefile | 2 +-
> t/meson.build | 2 +-
> t/unit-tests/t-reftable-basics.c | 219 -------------------------------
> t/unit-tests/u-reftable-basics.c | 195 +++++++++++++++++++++++++++
> 4 files changed, 197 insertions(+), 221 deletions(-)
> delete mode 100644 t/unit-tests/t-reftable-basics.c
> create mode 100644 t/unit-tests/u-reftable-basics.c
>
> diff --git a/Makefile b/Makefile
> index 13f9062a05..7b12bb078c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1362,6 +1362,7 @@ CLAR_TEST_SUITES += u-oid-array
> CLAR_TEST_SUITES += u-oidmap
> CLAR_TEST_SUITES += u-oidtree
> CLAR_TEST_SUITES += u-prio-queue
> +CLAR_TEST_SUITES += u-reftable-basics
> CLAR_TEST_SUITES += u-reftable-tree
> CLAR_TEST_SUITES += u-strbuf
> CLAR_TEST_SUITES += u-strcmp-offset
> @@ -1374,7 +1375,6 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
> CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
> CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
>
> -UNIT_TEST_PROGRAMS += t-reftable-basics
> UNIT_TEST_PROGRAMS += t-reftable-block
> UNIT_TEST_PROGRAMS += t-reftable-merged
> UNIT_TEST_PROGRAMS += t-reftable-pq
> diff --git a/t/meson.build b/t/meson.build
> index bfb744e886..8a42b595d9 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -8,6 +8,7 @@ clar_test_suites = [
> 'unit-tests/u-oidmap.c',
> 'unit-tests/u-oidtree.c',
> 'unit-tests/u-prio-queue.c',
> + 'unit-tests/u-reftable-basics.c',
> 'unit-tests/u-reftable-tree.c',
> 'unit-tests/u-strbuf.c',
> 'unit-tests/u-strcmp-offset.c',
> @@ -54,7 +55,6 @@ clar_unit_tests = executable('unit-tests',
> test('unit-tests', clar_unit_tests)
>
> unit_test_programs = [
> - 'unit-tests/t-reftable-basics.c',
> 'unit-tests/t-reftable-block.c',
> 'unit-tests/t-reftable-merged.c',
> 'unit-tests/t-reftable-pq.c',
> diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c
> deleted file mode 100644
> index c9e751e49e..0000000000
> --- a/t/unit-tests/t-reftable-basics.c
> +++ /dev/null
> @@ -1,219 +0,0 @@
> -/*
> -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 "test-lib.h"
> -#include "reftable/basics.h"
> -
> -struct integer_needle_lesseq_args {
> - int needle;
> - int *haystack;
> -};
> -
> -static int integer_needle_lesseq(size_t i, void *_args)
> -{
> - struct integer_needle_lesseq_args *args = _args;
> - return args->needle <= args->haystack[i];
> -}
> -
> -static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
> -{
> - return NULL;
> -}
> -
> -int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
> -{
> - if_test ("binary search with binsearch works") {
> - int haystack[] = { 2, 4, 6, 8, 10 };
> - struct {
> - int needle;
> - size_t expected_idx;
> - } testcases[] = {
> - {-9000, 0},
> - {-1, 0},
> - {0, 0},
> - {2, 0},
> - {3, 1},
> - {4, 1},
> - {7, 3},
> - {9, 4},
> - {10, 4},
> - {11, 5},
> - {9000, 5},
> - };
> -
> - for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) {
> - struct integer_needle_lesseq_args args = {
> - .haystack = haystack,
> - .needle = testcases[i].needle,
> - };
> - size_t idx;
> -
> - idx = binsearch(ARRAY_SIZE(haystack),
> - &integer_needle_lesseq, &args);
> - check_int(idx, ==, testcases[i].expected_idx);
> - }
> - }
> -
> - if_test ("names_length returns size of a NULL-terminated string array") {
> - const char *a[] = { "a", "b", NULL };
> - check_int(names_length(a), ==, 2);
> - }
> -
> - if_test ("names_equal compares NULL-terminated string arrays") {
> - const char *a[] = { "a", "b", "c", NULL };
> - const char *b[] = { "a", "b", "d", NULL };
> - const char *c[] = { "a", "b", NULL };
> -
> - check(names_equal(a, a));
> - check(!names_equal(a, b));
> - check(!names_equal(a, c));
> - }
> -
> - if_test ("parse_names works for basic input") {
> - char in1[] = "line\n";
> - char in2[] = "a\nb\nc";
> - char **out = parse_names(in1, strlen(in1));
> - check(out != NULL);
> - check_str(out[0], "line");
> - check(!out[1]);
> - free_names(out);
> -
> - out = parse_names(in2, strlen(in2));
> - check(out != NULL);
> - check_str(out[0], "a");
> - check_str(out[1], "b");
> - check_str(out[2], "c");
> - check(!out[3]);
> - free_names(out);
> - }
> -
> - if_test ("parse_names drops empty string") {
> - char in[] = "a\n\nb\n";
> - char **out = parse_names(in, strlen(in));
> - check(out != NULL);
> - check_str(out[0], "a");
> - /* simply '\n' should be dropped as empty string */
> - check_str(out[1], "b");
> - check(!out[2]);
> - free_names(out);
> - }
> -
> - if_test ("common_prefix_size works") {
> - struct reftable_buf a = REFTABLE_BUF_INIT;
> - struct reftable_buf b = REFTABLE_BUF_INIT;
> - struct {
> - const char *a, *b;
> - int want;
> - } cases[] = {
> - {"abcdef", "abc", 3},
> - { "abc", "ab", 2 },
> - { "", "abc", 0 },
> - { "abc", "abd", 2 },
> - { "abc", "pqr", 0 },
> - };
> -
> - for (size_t i = 0; i < ARRAY_SIZE(cases); i++) {
> - check(!reftable_buf_addstr(&a, cases[i].a));
> - check(!reftable_buf_addstr(&b, cases[i].b));
> - check_uint(common_prefix_size(&a, &b), ==, cases[i].want);
> - reftable_buf_reset(&a);
> - reftable_buf_reset(&b);
> - }
> - reftable_buf_release(&a);
> - reftable_buf_release(&b);
> - }
> -
> - if_test ("reftable_put_be64 and reftable_get_be64 work") {
> - uint64_t in = 0x1122334455667788;
> - uint8_t dest[8];
> - uint64_t out;
> - reftable_put_be64(dest, in);
> - out = reftable_get_be64(dest);
> - check_int(in, ==, out);
> - }
> -
> - if_test ("reftable_put_be32 and reftable_get_be32 work") {
> - uint32_t in = 0x11223344;
> - uint8_t dest[4];
> - uint32_t out;
> - reftable_put_be32(dest, in);
> - out = reftable_get_be32(dest);
> - check_int(in, ==, out);
> - }
> -
> - if_test ("reftable_put_be24 and reftable_get_be24 work") {
> - uint32_t in = 0x112233;
> - uint8_t dest[3];
> - uint32_t out;
> - reftable_put_be24(dest, in);
> - out = reftable_get_be24(dest);
> - check_int(in, ==, out);
> - }
> -
> - if_test ("put_be16 and get_be16 work") {
> - uint32_t in = 0xfef1;
> - uint8_t dest[3];
> - uint32_t out;
> - reftable_put_be16(dest, in);
> - out = reftable_get_be16(dest);
> - check_int(in, ==, out);
> - }
> -
> - if_test ("REFTABLE_ALLOC_GROW works") {
> - int *arr = NULL, *old_arr;
> - size_t alloc = 0, old_alloc;
> -
> - check(!REFTABLE_ALLOC_GROW(arr, 1, alloc));
> - check(arr != NULL);
> - check_uint(alloc, >=, 1);
> - arr[0] = 42;
> -
> - old_alloc = alloc;
> - old_arr = arr;
> - reftable_set_alloc(NULL, realloc_stub, NULL);
> - check(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> - check(arr == old_arr);
> - check_uint(alloc, ==, old_alloc);
> -
> - old_alloc = alloc;
> - reftable_set_alloc(NULL, NULL, NULL);
> - check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> - check(arr != NULL);
> - check_uint(alloc, >, old_alloc);
> - arr[alloc - 1] = 42;
> -
> - reftable_free(arr);
> - }
> -
> - if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
> - int *arr = NULL;
> - size_t alloc = 0, old_alloc;
> -
> - REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
> - check(arr != NULL);
> - check_uint(alloc, >=, 1);
> - arr[0] = 42;
> -
> - old_alloc = alloc;
> - REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> - check(arr != NULL);
> - check_uint(alloc, >, old_alloc);
> - arr[alloc - 1] = 42;
> -
> - old_alloc = alloc;
> - reftable_set_alloc(NULL, realloc_stub, NULL);
> - REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> - check(arr == NULL);
> - check_uint(alloc, ==, 0);
> - reftable_set_alloc(NULL, NULL, NULL);
> -
> - reftable_free(arr);
> - }
> -
> - return test_done();
> -}
> diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
> new file mode 100644
> index 0000000000..63dd568faf
> --- /dev/null
> +++ b/t/unit-tests/u-reftable-basics.c
> @@ -0,0 +1,195 @@
> +/*
> +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/basics.h"
> +
> +struct integer_needle_lesseq_args {
> + int needle;
> + int *haystack;
> +};
> +
> +static int integer_needle_lesseq(size_t i, void *_args)
> +{
> + struct integer_needle_lesseq_args *args = _args;
> + return args->needle <= args->haystack[i];
> +}
> +
> +static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
> +{
> + return NULL;
> +}
> +
> +void test_reftable_basics__binsearch(void)
> +{
> + int haystack[] = { 2, 4, 6, 8, 10 };
> + struct {
> + int needle;
> + size_t expected_idx;
> + } testcases[] = {
> + {-9000, 0},
> + {-1, 0},
> + {0, 0},
> + {2, 0},
> + {3, 1},
> + {4, 1},
> + {7, 3},
> + {9, 4},
> + {10, 4},
> + {11, 5},
> + {9000, 5},
> + };
> +
> + for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) {
> + struct integer_needle_lesseq_args args = {
> + .haystack = haystack,
> + .needle = testcases[i].needle,
> + };
> + size_t idx;
> +
> + idx = binsearch(ARRAY_SIZE(haystack),
> + &integer_needle_lesseq, &args);
> + cl_assert_equal_i(idx, testcases[i].expected_idx);
> + }
> +
Nit: empty newline can be removed.
> +}
> +
> +void test_reftable_basics__names_length(void)
> +{
> + const char *a[] = { "a", "b", NULL };
> + cl_assert_equal_i(names_length(a), 2);
> +}
> +
> +void test_reftable_basics__names_equal(void)
> +{
> + const char *a[] = { "a", "b", "c", NULL };
> + const char *b[] = { "a", "b", "d", NULL };
> + const char *c[] = { "a", "b", NULL };
> +
> + cl_assert(names_equal(a, a));
> + cl_assert(!names_equal(a, b));
> + cl_assert(!names_equal(a, c));
> +}
> +
> +void test_reftable_basics__parse_names(void)
> +{
> + char in1[] = "line\n";
> + char in2[] = "a\nb\nc";
> + char **out = parse_names(in1, strlen(in1));
> + cl_assert(out != NULL);
> + cl_assert_equal_s(out[0], "line");
> + cl_assert(!out[1]);
> + free_names(out);
> +
> + out = parse_names(in2, strlen(in2));
> + cl_assert(out != NULL);
> + cl_assert_equal_s(out[0], "a");
> + cl_assert_equal_s(out[1], "b");
> + cl_assert_equal_s(out[2], "c");
> + cl_assert(!out[3]);
> + free_names(out);
> +}
> +
I think you missed converting "parse_names drops empty string".
> +void test_reftable_basics__common_prefix_size(void)
> +{
> + struct reftable_buf a = REFTABLE_BUF_INIT;
> + struct reftable_buf b = REFTABLE_BUF_INIT;
> + struct {
> + const char *a, *b;
> + int want;
> + } cases[] = {
> + {"abcdef", "abc", 3},
> + { "abc", "ab", 2 },
> + { "", "abc", 0 },
> + { "abc", "abd", 2 },
> + { "abc", "pqr", 0 },
> + };
> +
> + for (size_t i = 0; i < ARRAY_SIZE(cases); i++) {
> + reftable_buf_reset(&a);
> + reftable_buf_reset(&b);
> + cl_assert_equal_i(reftable_buf_addstr(&a, cases[i].a), 0);
> + cl_assert_equal_i(reftable_buf_addstr(&b, cases[i].b), 0);
> + cl_assert_equal_i(common_prefix_size(&a, &b), cases[i].want);
Why did you change the order of `reftable_buf_reset()` calls? The
reordered logic achieves the same result, but I'd recommend to keep
things as-is so that reviewers aren't puzzled by this arbitrary change.
> + }
> + reftable_buf_release(&a);
> + reftable_buf_release(&b);
> +}
I miss tests for `put_be64` and `put_be32`.
> +void test_reftable_basics__put_get_be24(void)
> +{
> + uint32_t in = 0x112233;
> + uint8_t dest[3];
> + uint32_t out;
> + reftable_put_be24(dest, in);
> + out = reftable_get_be24(dest);
> + cl_assert_equal_i(in, out);
> +}
> +
> +void test_reftable_basics__put_get_be16(void)
> +{
> + uint32_t in = 0xfef1;
> + uint8_t dest[3];
> + uint32_t out;
> + reftable_put_be16(dest, in);
> + out = reftable_get_be16(dest);
> + cl_assert_equal_i(in, out);
> +}
> +
> +void test_reftable_basics__grow_alloc(void)
Let's rename this to `__alloc_grow()` to match the name of the function.
> +{
> + int *arr = NULL, *old_arr;
> + size_t alloc = 0, old_alloc;
> +
> + cl_assert_equal_i(REFTABLE_ALLOC_GROW(arr, 1, alloc), 0);
> + cl_assert(arr != NULL);
> + cl_assert(alloc >= 1);
> + arr[0] = 42;
> +
> + old_alloc = alloc;
> + old_arr = arr;
> + reftable_set_alloc(NULL, realloc_stub, NULL);
> + cl_assert(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
> + cl_assert(arr == old_arr);
> + cl_assert_equal_i(alloc, old_alloc);
> +
> + old_alloc = alloc;
> + reftable_set_alloc(NULL, NULL, NULL);
> + cl_assert_equal_i(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc), 0);
> + cl_assert(arr != NULL);
> + cl_assert(alloc > old_alloc);
> + arr[alloc - 1] = 42;
> +
> + reftable_free(arr);
> +}
> +
> +void test_reftable_basics__grow_alloc_or_null(void)
Same here, let's rename to `alloc_grow_or_null`.
> +{
> + int *arr = NULL;
> + size_t alloc = 0, old_alloc;
> +
> + REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
> + cl_assert(arr != NULL);
> + cl_assert(alloc >= 1);
> + arr[0] = 42;
> +
> + old_alloc = alloc;
> + REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> + cl_assert(arr != NULL);
> + cl_assert(alloc > old_alloc);
> + arr[alloc - 1] = 42;
> +
> + old_alloc = alloc;
> + reftable_set_alloc(NULL, realloc_stub, NULL);
> + REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> + cl_assert(arr == NULL);
> + cl_assert_equal_i(alloc, 0);
> + reftable_set_alloc(NULL, NULL, NULL);
> +
> + reftable_free(arr);
> +}
Other than that this looks well-done to me, thanks!
Patrick
