| Message ID | 20240703034638.8019-2-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSoC] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c | expand |
Hi Ghanshyam Overall this looks like a faithful conversion, I've left a few comments below. On 03/07/2024 04:46, Ghanshyam Thakkar wrote: > helper/test-oid-array.c along with t0064-oid-array.sh test the > oid-array.h library, which provides storage and processing > efficiency over large lists of object identifiers. > > Migrate them to the unit testing framework for better runtime > performance and efficiency. Also 'the_hash_algo' is used internally in > oid_array_lookup(), but we do not initialize a repository directory, > therefore initialize the_hash_algo manually. And > init_hash_algo():lib-oid.c can aid in this process, so make it public. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to > replace these internal function used in TEST_LOOKUP() with TEST_RUN(). Nice idea > diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c > index 37105f0a8f..8f0ccac532 100644 > --- a/t/unit-tests/lib-oid.c > +++ b/t/unit-tests/lib-oid.c > @@ -3,7 +3,7 @@ > #include "strbuf.h" > #include "hex.h" > > -static int init_hash_algo(void) > +int init_hash_algo(void) > { > static int algo = -1; > > diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h > index 8d2acca768..011a2d88de 100644 > --- a/t/unit-tests/lib-oid.h > +++ b/t/unit-tests/lib-oid.h > @@ -13,5 +13,11 @@ > * environment variable. > */ > int get_oid_arbitrary_hex(const char *s, struct object_id *oid); > +/* > + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of > + * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of > + * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. > + */ > +int init_hash_algo(void); > > #endif /* LIB_OID_H */ > diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c > new file mode 100644 > index 0000000000..0a506fab07 > --- /dev/null > +++ b/t/unit-tests/t-oid-array.c > @@ -0,0 +1,138 @@ > +#define USE_THE_REPOSITORY_VARIABLE > + > +#include "test-lib.h" > +#include "lib-oid.h" > +#include "oid-array.h" > +#include "hex.h" > + > +static inline int test_min(int a, int b) > +{ > + return a <= b ? a : b; > +} > + > +static int fill_array(struct oid_array *array, const char *hexes[], size_t n) > +{ > + for (size_t i = 0; i < n; i++) { > + struct object_id oid; > + > + if (get_oid_arbitrary_hex(hexes[i], &oid)) > + return -1; > + oid_array_append(array, &oid); > + } > + if (!check_int(array->nr, ==, n)) This should probably use check_uint() as the arguments are unsigned integers. > + return -1; > + return 0; > +} > + > +static int add_to_oid_array(const struct object_id *oid, void *data) > +{ > + struct oid_array *array = data; style: we leave a blank line after variable declarations at the start of a block. > + oid_array_append(array, oid); > + return 0; > +} > + > +static void t_enumeration(const char *input_args[], size_t input_sz, > + const char *result[], size_t result_sz) style: we use "const char **arg" rather than "const char *arg[]" > +{ > + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT, > + actual = OID_ARRAY_INIT; > + size_t i; > + > + if (fill_array(&input, input_args, input_sz)) > + return; > + if (fill_array(&expect, result, result_sz)) > + return; > + > + oid_array_for_each_unique(&input, add_to_oid_array, &actual); > + check_int(actual.nr, ==, expect.nr); > + > + for (i = 0; i < test_min(actual.nr, expect.nr); i++) { > + if (!check(oideq(&actual.oid[i], &expect.oid[i]))) > + test_msg("expected: %s\n got: %s\n index: %" PRIuMAX, > + oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]), > + (uintmax_t)i); > + } > + check_int(i, ==, result_sz); > + > + oid_array_clear(&actual); > + oid_array_clear(&input); > + oid_array_clear(&expect); > +} > + > +#define TEST_ENUMERATION(input, result, desc) \ > + TEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \ > + desc " works") This macro and its helper function look good. > +static int t_lookup(struct object_id *oid_query, const char *query, > + const char *hexes[], size_t n) > +{ > + struct oid_array array = OID_ARRAY_INIT; > + int ret; > + > + if (get_oid_arbitrary_hex(query, oid_query)) > + return INT_MIN; > + if (fill_array(&array, hexes, n)) > + return INT_MIN; > + ret = oid_array_lookup(&array, oid_query); > + > + oid_array_clear(&array); > + return ret; > +} > + > +#define TEST_LOOKUP(input_args, query, condition, desc) \ Passing in the condition is a bit unfortunate as it means that the caller has to know which variable name to use. It might be nicer to have a function instead that takes the upper and lower bounds of the expected result and then does check_int(res, >=, expected_lower); check_int(res, <=, expected_upper); It might be worth checking that array[res] matches the expected entry as well. > + do { \ > + int skip = test__run_begin(); \ > + if (!skip) { \ > + struct object_id oid_query; \ > + int ret = t_lookup(&oid_query, query, input_args, \ > + ARRAY_SIZE(input_args)); \ > + \ > + if (ret != INT_MIN && !check(condition)) \ > + test_msg("oid query for lookup: %s", \ > + oid_to_hex(&oid_query)); \ > + } \ > + test__run_end(!skip, TEST_LOCATION(), desc " works"); \ > + } while (0) > + > +static void setup(void) > +{ > + int algo = init_hash_algo(); > + /* because the_hash_algo is used by oid_array_lookup() internally */ > + if (check_int(algo, !=, GIT_HASH_UNKNOWN)) > + repo_set_hash_algo(the_repository, algo); > +} > + > +int cmd_main(int argc UNUSED, const char **argv UNUSED) > +{ > + const char *arr_input[] = { "88", "44", "aa", "55" }; > + const char *arr_input_dup[] = { "88", "44", "aa", "55", > + "88", "44", "aa", "55", > + "88", "44", "aa", "55" }; > + const char *res_sorted[] = { "44", "55", "88", "aa" }; > + > + if (!TEST(setup(), "setup")) > + test_skip_all("hash algo initialization failed"); > + > + TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration"); > + TEST_ENUMERATION(arr_input_dup, res_sorted, > + "ordered enumeration with duplicate suppression"); > + > + /* ret is the return value of oid_array_lookup() */ > + TEST_LOOKUP(arr_input, "55", ret == 1, "lookup"); > + TEST_LOOKUP(arr_input, "33", ret < 0, "lookup non-existent entry"); > + TEST_LOOKUP(arr_input_dup, "55", ret >= 3 && ret <= 5, > + "lookup with duplicates"); > + TEST_LOOKUP(arr_input_dup, "66", ret < 0, > + "lookup non-existent entry with duplicates"); > + > + TEST_LOOKUP(((const char *[]){ > + "55", > + init_hash_algo() == GIT_HASH_SHA1 ? > + "5500000000000000000000000000000000000001" : > + "5500000000000000000000000000000000000000000000000000000000000001" }), > + "55", ret == 0, "lookup with almost duplicate values"); This might be slightly more readable if we stored the oids in a separate variable at the beginning of this function and then it would look something like TEST_LOOKUP(((const char *[]) {"55", nearly_55}), "55", 0, 0, "lookup with almost duplicate values"); Having said that it is kind of unfortunate that we have all the variable definitions at the start as it makes it harder to see what's going on in each test. We could avoid that by using TEST_RUN() and declaring the variables in the test block. For example the first test would look like TEST_RUN("ordered enumeration") { const char *input[] = { "88", "44", "aa", "55" }; const char *expected[] = { "44", "55", "88", "aa" }; TEST_ENUMERATION(input, expected) } where TEST_ENUMERATION is adjusted so it does not call TEST(). It's more verbose but it is clearer what the input and expected values actually are. Best Wishes Phillip > + TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", > + ret >= 0 && ret <= 1, "lookup with single duplicate value"); > + > + return test_done(); > +}
Hi Phillip, Phillip Wood <phillip.wood123@gmail.com> wrote: > On 03/07/2024 04:46, Ghanshyam Thakkar wrote: > > Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to > > replace these internal function used in TEST_LOOKUP() with TEST_RUN(). > > Nice idea I think the consensus on the 'TEST_RUN()' (which is now 'if_test()') would take a bit more time and since the v2 removes the use of internal functions anyways, I think we should not have to wait for 'if_test()' anymore (other things like declaring input varibles inside the 'if_test()' block can be addressed as an incremental patch once 'if_test()' gets merged). And I've addressed all your other review points in v2 as well. Link to v2: https://lore.kernel.org/git/20240803132206.72166-1-shyamthakkar001@gmail.com/ Thanks.
diff --git a/Makefile b/Makefile index 3eab701b10..26a521c027 100644 --- a/Makefile +++ b/Makefile @@ -808,7 +808,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o TEST_BUILTINS_OBJS += test-match-trees.o TEST_BUILTINS_OBJS += test-mergesort.o TEST_BUILTINS_OBJS += test-mktemp.o -TEST_BUILTINS_OBJS += test-oid-array.o TEST_BUILTINS_OBJS += test-oidmap.o TEST_BUILTINS_OBJS += test-online-cpus.o TEST_BUILTINS_OBJS += test-pack-mtimes.o @@ -1337,6 +1336,7 @@ UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-example-decorate UNIT_TEST_PROGRAMS += t-hash UNIT_TEST_PROGRAMS += t-mem-pool +UNIT_TEST_PROGRAMS += t-oid-array UNIT_TEST_PROGRAMS += t-oidtree UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-reftable-basics diff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c deleted file mode 100644 index 076b849cbf..0000000000 --- a/t/helper/test-oid-array.c +++ /dev/null @@ -1,49 +0,0 @@ -#define USE_THE_REPOSITORY_VARIABLE - -#include "test-tool.h" -#include "hex.h" -#include "oid-array.h" -#include "setup.h" -#include "strbuf.h" - -static int print_oid(const struct object_id *oid, void *data UNUSED) -{ - puts(oid_to_hex(oid)); - return 0; -} - -int cmd__oid_array(int argc UNUSED, const char **argv UNUSED) -{ - struct oid_array array = OID_ARRAY_INIT; - struct strbuf line = STRBUF_INIT; - int nongit_ok; - - setup_git_directory_gently(&nongit_ok); - if (nongit_ok) - repo_set_hash_algo(the_repository, GIT_HASH_SHA1); - - while (strbuf_getline(&line, stdin) != EOF) { - const char *arg; - struct object_id oid; - - if (skip_prefix(line.buf, "append ", &arg)) { - if (get_oid_hex(arg, &oid)) - die("not a hexadecimal oid: %s", arg); - oid_array_append(&array, &oid); - } else if (skip_prefix(line.buf, "lookup ", &arg)) { - if (get_oid_hex(arg, &oid)) - die("not a hexadecimal oid: %s", arg); - printf("%d\n", oid_array_lookup(&array, &oid)); - } else if (!strcmp(line.buf, "clear")) - oid_array_clear(&array); - else if (!strcmp(line.buf, "for_each_unique")) - oid_array_for_each_unique(&array, print_oid, NULL); - else - die("unknown command: %s", line.buf); - } - - strbuf_release(&line); - oid_array_clear(&array); - - return 0; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 93436a82ae..fdbf755fb0 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -43,7 +43,6 @@ static struct test_cmd cmds[] = { { "match-trees", cmd__match_trees }, { "mergesort", cmd__mergesort }, { "mktemp", cmd__mktemp }, - { "oid-array", cmd__oid_array }, { "oidmap", cmd__oidmap }, { "online-cpus", cmd__online_cpus }, { "pack-mtimes", cmd__pack_mtimes }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index d9033d14e1..0d9b9f6583 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -65,7 +65,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv); int cmd__serve_v2(int argc, const char **argv); int cmd__sha1(int argc, const char **argv); int cmd__sha1_is_sha1dc(int argc, const char **argv); -int cmd__oid_array(int argc, const char **argv); int cmd__sha256(int argc, const char **argv); int cmd__sigchain(int argc, const char **argv); int cmd__simple_ipc(int argc, const char **argv); diff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh deleted file mode 100755 index de74b692d0..0000000000 --- a/t/t0064-oid-array.sh +++ /dev/null @@ -1,122 +0,0 @@ -#!/bin/sh - -test_description='basic tests for the oid array implementation' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -echoid () { - prefix="${1:+$1 }" - shift - while test $# -gt 0 - do - echo "$prefix$ZERO_OID" | sed -e "s/00/$1/g" - shift - done -} - -test_expect_success 'without repository' ' - cat >expect <<-EOF && - 4444444444444444444444444444444444444444 - 5555555555555555555555555555555555555555 - 8888888888888888888888888888888888888888 - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - EOF - cat >input <<-EOF && - append 4444444444444444444444444444444444444444 - append 5555555555555555555555555555555555555555 - append 8888888888888888888888888888888888888888 - append aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - for_each_unique - EOF - nongit test-tool oid-array <input >actual && - test_cmp expect actual -' - -test_expect_success 'ordered enumeration' ' - echoid "" 44 55 88 aa >expect && - { - echoid append 88 44 aa 55 && - echo for_each_unique - } | test-tool oid-array >actual && - test_cmp expect actual -' - -test_expect_success 'ordered enumeration with duplicate suppression' ' - echoid "" 44 55 88 aa >expect && - { - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echo for_each_unique - } | test-tool oid-array >actual && - test_cmp expect actual -' - -test_expect_success 'lookup' ' - { - echoid append 88 44 aa 55 && - echoid lookup 55 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -eq 1 -' - -test_expect_success 'lookup non-existing entry' ' - { - echoid append 88 44 aa 55 && - echoid lookup 33 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -lt 0 -' - -test_expect_success 'lookup with duplicates' ' - { - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echoid lookup 55 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -ge 3 && - test "$n" -le 5 -' - -test_expect_success 'lookup non-existing entry with duplicates' ' - { - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echoid append 88 44 aa 55 && - echoid lookup 66 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -lt 0 -' - -test_expect_success 'lookup with almost duplicate values' ' - # n-1 5s - root=$(echoid "" 55) && - root=${root%5} && - { - id1="${root}5" && - id2="${root}f" && - echo "append $id1" && - echo "append $id2" && - echoid lookup 55 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -eq 0 -' - -test_expect_success 'lookup with single duplicate value' ' - { - echoid append 55 55 && - echoid lookup 55 - } | test-tool oid-array >actual && - n=$(cat actual) && - test "$n" -ge 0 && - test "$n" -le 1 -' - -test_done diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c index 37105f0a8f..8f0ccac532 100644 --- a/t/unit-tests/lib-oid.c +++ b/t/unit-tests/lib-oid.c @@ -3,7 +3,7 @@ #include "strbuf.h" #include "hex.h" -static int init_hash_algo(void) +int init_hash_algo(void) { static int algo = -1; diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h index 8d2acca768..011a2d88de 100644 --- a/t/unit-tests/lib-oid.h +++ b/t/unit-tests/lib-oid.h @@ -13,5 +13,11 @@ * environment variable. */ int get_oid_arbitrary_hex(const char *s, struct object_id *oid); +/* + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of + * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of + * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. + */ +int init_hash_algo(void); #endif /* LIB_OID_H */ diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c new file mode 100644 index 0000000000..0a506fab07 --- /dev/null +++ b/t/unit-tests/t-oid-array.c @@ -0,0 +1,138 @@ +#define USE_THE_REPOSITORY_VARIABLE + +#include "test-lib.h" +#include "lib-oid.h" +#include "oid-array.h" +#include "hex.h" + +static inline int test_min(int a, int b) +{ + return a <= b ? a : b; +} + +static int fill_array(struct oid_array *array, const char *hexes[], size_t n) +{ + for (size_t i = 0; i < n; i++) { + struct object_id oid; + + if (get_oid_arbitrary_hex(hexes[i], &oid)) + return -1; + oid_array_append(array, &oid); + } + if (!check_int(array->nr, ==, n)) + return -1; + return 0; +} + +static int add_to_oid_array(const struct object_id *oid, void *data) +{ + struct oid_array *array = data; + oid_array_append(array, oid); + return 0; +} + +static void t_enumeration(const char *input_args[], size_t input_sz, + const char *result[], size_t result_sz) +{ + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT, + actual = OID_ARRAY_INIT; + size_t i; + + if (fill_array(&input, input_args, input_sz)) + return; + if (fill_array(&expect, result, result_sz)) + return; + + oid_array_for_each_unique(&input, add_to_oid_array, &actual); + check_int(actual.nr, ==, expect.nr); + + for (i = 0; i < test_min(actual.nr, expect.nr); i++) { + if (!check(oideq(&actual.oid[i], &expect.oid[i]))) + test_msg("expected: %s\n got: %s\n index: %" PRIuMAX, + oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]), + (uintmax_t)i); + } + check_int(i, ==, result_sz); + + oid_array_clear(&actual); + oid_array_clear(&input); + oid_array_clear(&expect); +} + +#define TEST_ENUMERATION(input, result, desc) \ + TEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \ + desc " works") + +static int t_lookup(struct object_id *oid_query, const char *query, + const char *hexes[], size_t n) +{ + struct oid_array array = OID_ARRAY_INIT; + int ret; + + if (get_oid_arbitrary_hex(query, oid_query)) + return INT_MIN; + if (fill_array(&array, hexes, n)) + return INT_MIN; + ret = oid_array_lookup(&array, oid_query); + + oid_array_clear(&array); + return ret; +} + +#define TEST_LOOKUP(input_args, query, condition, desc) \ + do { \ + int skip = test__run_begin(); \ + if (!skip) { \ + struct object_id oid_query; \ + int ret = t_lookup(&oid_query, query, input_args, \ + ARRAY_SIZE(input_args)); \ + \ + if (ret != INT_MIN && !check(condition)) \ + test_msg("oid query for lookup: %s", \ + oid_to_hex(&oid_query)); \ + } \ + test__run_end(!skip, TEST_LOCATION(), desc " works"); \ + } while (0) + +static void setup(void) +{ + int algo = init_hash_algo(); + /* because the_hash_algo is used by oid_array_lookup() internally */ + if (check_int(algo, !=, GIT_HASH_UNKNOWN)) + repo_set_hash_algo(the_repository, algo); +} + +int cmd_main(int argc UNUSED, const char **argv UNUSED) +{ + const char *arr_input[] = { "88", "44", "aa", "55" }; + const char *arr_input_dup[] = { "88", "44", "aa", "55", + "88", "44", "aa", "55", + "88", "44", "aa", "55" }; + const char *res_sorted[] = { "44", "55", "88", "aa" }; + + if (!TEST(setup(), "setup")) + test_skip_all("hash algo initialization failed"); + + TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration"); + TEST_ENUMERATION(arr_input_dup, res_sorted, + "ordered enumeration with duplicate suppression"); + + /* ret is the return value of oid_array_lookup() */ + TEST_LOOKUP(arr_input, "55", ret == 1, "lookup"); + TEST_LOOKUP(arr_input, "33", ret < 0, "lookup non-existent entry"); + TEST_LOOKUP(arr_input_dup, "55", ret >= 3 && ret <= 5, + "lookup with duplicates"); + TEST_LOOKUP(arr_input_dup, "66", ret < 0, + "lookup non-existent entry with duplicates"); + + TEST_LOOKUP(((const char *[]){ + "55", + init_hash_algo() == GIT_HASH_SHA1 ? + "5500000000000000000000000000000000000001" : + "5500000000000000000000000000000000000000000000000000000000000001" }), + "55", ret == 0, "lookup with almost duplicate values"); + TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", + ret >= 0 && ret <= 1, "lookup with single duplicate value"); + + return test_done(); +}
helper/test-oid-array.c along with t0064-oid-array.sh test the oid-array.h library, which provides storage and processing efficiency over large lists of object identifiers. Migrate them to the unit testing framework for better runtime performance and efficiency. Also 'the_hash_algo' is used internally in oid_array_lookup(), but we do not initialize a repository directory, therefore initialize the_hash_algo manually. And init_hash_algo():lib-oid.c can aid in this process, so make it public. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to replace these internal function used in TEST_LOOKUP() with TEST_RUN(). Makefile | 2 +- t/helper/test-oid-array.c | 49 ------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0064-oid-array.sh | 122 -------------------------------- t/unit-tests/lib-oid.c | 2 +- t/unit-tests/lib-oid.h | 6 ++ t/unit-tests/t-oid-array.c | 138 +++++++++++++++++++++++++++++++++++++ 8 files changed, 146 insertions(+), 175 deletions(-) delete mode 100644 t/helper/test-oid-array.c delete mode 100755 t/t0064-oid-array.sh create mode 100644 t/unit-tests/t-oid-array.c