| Message ID | 20240619175036.64291-1-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [GSoC] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c | expand |
Ghanshyam Thakkar wrote: > helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h > library which is built on top of hashmap.h to store arbitrary > datastructure (which must contain oidmap_entry, which is a wrapper > around object_id). These entries can be accessed by querying their > associated object_id. > > Migrate them to the unit testing framework for better performance, > concise code and better debugging. Along with the migration also plug > memory leaks and make the test logic independent for all the tests. > The migration removes 'put' tests from t0016, because it is used as > setup to all the other tests, so testing it separately does not yield > any benefit. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > This patch is depenedent on 'gt/unit-test-oidtree' for lib-oid. Very neat! I'm cc-ing Josh Steadmon for unit test framework expertise. Patch left unsnipped for reference. > Makefile | 2 +- > t/helper/test-oidmap.c | 123 ------------------------------ > t/helper/test-tool.c | 1 - > t/helper/test-tool.h | 1 - > t/t0016-oidmap.sh | 112 --------------------------- > t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 166 insertions(+), 238 deletions(-) > delete mode 100644 t/helper/test-oidmap.c > delete mode 100755 t/t0016-oidmap.sh > create mode 100644 t/unit-tests/t-oidmap.c > > diff --git a/Makefile b/Makefile > index 03751e0fc0..f7ed50f3a9 100644 > --- a/Makefile > +++ b/Makefile > @@ -810,7 +810,6 @@ 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 > TEST_BUILTINS_OBJS += test-parse-options.o > @@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/% > > UNIT_TEST_PROGRAMS += t-ctype > UNIT_TEST_PROGRAMS += t-mem-pool > +UNIT_TEST_PROGRAMS += t-oidmap > UNIT_TEST_PROGRAMS += t-oidtree > UNIT_TEST_PROGRAMS += t-prio-queue > UNIT_TEST_PROGRAMS += t-strbuf > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c > deleted file mode 100644 > index bd30244a54..0000000000 > --- a/t/helper/test-oidmap.c > +++ /dev/null > @@ -1,123 +0,0 @@ > -#include "test-tool.h" > -#include "hex.h" > -#include "object-name.h" > -#include "oidmap.h" > -#include "repository.h" > -#include "setup.h" > -#include "strbuf.h" > -#include "string-list.h" > - > -/* key is an oid and value is a name (could be a refname for example) */ > -struct test_entry { > - struct oidmap_entry entry; > - char name[FLEX_ARRAY]; > -}; > - > -#define DELIM " \t\r\n" > - > -/* > - * Read stdin line by line and print result of commands to stdout: > - * > - * hash oidkey -> sha1hash(oidkey) > - * put oidkey namevalue -> NULL / old namevalue > - * get oidkey -> NULL / namevalue > - * remove oidkey -> NULL / old namevalue > - * iterate -> oidkey1 namevalue1\noidkey2 namevalue2\n... > - * > - */ > -int cmd__oidmap(int argc UNUSED, const char **argv UNUSED) > -{ > - struct string_list parts = STRING_LIST_INIT_NODUP; > - struct strbuf line = STRBUF_INIT; > - struct oidmap map = OIDMAP_INIT; > - > - setup_git_directory(); > - > - /* init oidmap */ > - oidmap_init(&map, 0); > - > - /* process commands from stdin */ > - while (strbuf_getline(&line, stdin) != EOF) { > - char *cmd, *p1, *p2; > - struct test_entry *entry; > - struct object_id oid; > - > - /* break line into command and up to two parameters */ > - string_list_setlen(&parts, 0); > - string_list_split_in_place(&parts, line.buf, DELIM, 2); > - string_list_remove_empty_items(&parts, 0); > - > - /* ignore empty lines */ > - if (!parts.nr) > - continue; > - if (!*parts.items[0].string || *parts.items[0].string == '#') > - continue; > - > - cmd = parts.items[0].string; > - p1 = parts.nr >= 1 ? parts.items[1].string : NULL; > - p2 = parts.nr >= 2 ? parts.items[2].string : NULL; > - > - if (!strcmp("put", cmd) && p1 && p2) { > - > - if (repo_get_oid(the_repository, p1, &oid)) { > - printf("Unknown oid: %s\n", p1); > - continue; > - } > - > - /* create entry with oid_key = p1, name_value = p2 */ > - FLEX_ALLOC_STR(entry, name, p2); > - oidcpy(&entry->entry.oid, &oid); > - > - /* add / replace entry */ > - entry = oidmap_put(&map, entry); > - > - /* print and free replaced entry, if any */ > - puts(entry ? entry->name : "NULL"); > - free(entry); > - > - } else if (!strcmp("get", cmd) && p1) { > - > - if (repo_get_oid(the_repository, p1, &oid)) { > - printf("Unknown oid: %s\n", p1); > - continue; > - } > - > - /* lookup entry in oidmap */ > - entry = oidmap_get(&map, &oid); > - > - /* print result */ > - puts(entry ? entry->name : "NULL"); > - > - } else if (!strcmp("remove", cmd) && p1) { > - > - if (repo_get_oid(the_repository, p1, &oid)) { > - printf("Unknown oid: %s\n", p1); > - continue; > - } > - > - /* remove entry from oidmap */ > - entry = oidmap_remove(&map, &oid); > - > - /* print result and free entry*/ > - puts(entry ? entry->name : "NULL"); > - free(entry); > - > - } else if (!strcmp("iterate", cmd)) { > - > - struct oidmap_iter iter; > - oidmap_iter_init(&map, &iter); > - while ((entry = oidmap_iter_next(&iter))) > - printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); > - > - } else { > - > - printf("Unknown command %s\n", cmd); > - > - } > - } > - > - string_list_clear(&parts, 0); > - strbuf_release(&line); > - oidmap_free(&map, 1); > - return 0; > -} > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index 253324a06b..5f013d8b2b 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -45,7 +45,6 @@ static struct test_cmd cmds[] = { > { "mergesort", cmd__mergesort }, > { "mktemp", cmd__mktemp }, > { "oid-array", cmd__oid_array }, > - { "oidmap", cmd__oidmap }, > { "online-cpus", cmd__online_cpus }, > { "pack-mtimes", cmd__pack_mtimes }, > { "parse-options", cmd__parse_options }, > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index 460dd7d260..c7d3e43694 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv); > int cmd__match_trees(int argc, const char **argv); > int cmd__mergesort(int argc, const char **argv); > int cmd__mktemp(int argc, const char **argv); > -int cmd__oidmap(int argc, const char **argv); > int cmd__online_cpus(int argc, const char **argv); > int cmd__pack_mtimes(int argc, const char **argv); > int cmd__parse_options(int argc, const char **argv); > diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh > deleted file mode 100755 > index 0faef1f4f1..0000000000 > --- a/t/t0016-oidmap.sh > +++ /dev/null > @@ -1,112 +0,0 @@ > -#!/bin/sh > - > -test_description='test oidmap' > - > -TEST_PASSES_SANITIZE_LEAK=true > -. ./test-lib.sh > - > -# This purposefully is very similar to t0011-hashmap.sh > - > -test_oidmap () { > - echo "$1" | test-tool oidmap $3 >actual && > - echo "$2" >expect && > - test_cmp expect actual > -} > - > - > -test_expect_success 'setup' ' > - > - test_commit one && > - test_commit two && > - test_commit three && > - test_commit four > - > -' > - > -test_expect_success 'put' ' > - > -test_oidmap "put one 1 > -put two 2 > -put invalidOid 4 > -put three 3" "NULL > -NULL > -Unknown oid: invalidOid > -NULL" > - > -' > - > -test_expect_success 'replace' ' > - > -test_oidmap "put one 1 > -put two 2 > -put three 3 > -put invalidOid 4 > -put two deux > -put one un" "NULL > -NULL > -NULL > -Unknown oid: invalidOid > -2 > -1" > - > -' > - > -test_expect_success 'get' ' > - > -test_oidmap "put one 1 > -put two 2 > -put three 3 > -get two > -get four > -get invalidOid > -get one" "NULL > -NULL > -NULL > -2 > -NULL > -Unknown oid: invalidOid > -1" > - > -' > - > -test_expect_success 'remove' ' > - > -test_oidmap "put one 1 > -put two 2 > -put three 3 > -remove one > -remove two > -remove invalidOid > -remove four" "NULL > -NULL > -NULL > -1 > -2 > -Unknown oid: invalidOid > -NULL" > - > -' > - > -test_expect_success 'iterate' ' > - test-tool oidmap >actual.raw <<-\EOF && > - put one 1 > - put two 2 > - put three 3 > - iterate > - EOF > - > - # sort "expect" too so we do not rely on the order of particular oids > - sort >expect <<-EOF && > - NULL > - NULL > - NULL > - $(git rev-parse one) 1 > - $(git rev-parse two) 2 > - $(git rev-parse three) 3 > - EOF > - > - sort <actual.raw >actual && > - test_cmp expect actual > -' > - > -test_done > diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c > new file mode 100644 > index 0000000000..9b98a3ed09 > --- /dev/null > +++ b/t/unit-tests/t-oidmap.c > @@ -0,0 +1,165 @@ > +#include "test-lib.h" > +#include "lib-oid.h" > +#include "oidmap.h" > +#include "hash.h" > +#include "hex.h" > + > +/* > + * elements we will put in oidmap structs are made of a key: the entry.oid > + * field, which is of type struct object_id, and a value: the name field (could > + * be a refname for example) > + */ > +struct test_entry { > + struct oidmap_entry entry; > + char name[FLEX_ARRAY]; > +}; > + > +static const char *key_val[][2] = { { "11", "one" }, > + { "22", "two" }, > + { "33", "three" } }; > + > +static int put_and_check_null(struct oidmap *map, const char *hex, > + const char *entry_name) > +{ > + struct test_entry *entry; > + > + FLEX_ALLOC_STR(entry, name, entry_name); > + if (get_oid_arbitrary_hex(hex, &entry->entry.oid)) > + return -1; > + if (!check(oidmap_put(map, entry) == NULL)) > + return -1; > + return 0; > +} > + > +static void setup(void (*f)(struct oidmap *map)) > +{ > + struct oidmap map = OIDMAP_INIT; > + int ret = 0; > + > + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) > + if ((ret = put_and_check_null(&map, key_val[i][0], > + key_val[i][1]))) > + break; > + > + if (!ret) > + f(&map); > + oidmap_free(&map, 1); > +} > + > +static void t_replace(struct oidmap *map) > +{ > + struct test_entry *entry, *prev; > + > + FLEX_ALLOC_STR(entry, name, "un"); > + if (get_oid_arbitrary_hex("11", &entry->entry.oid)) > + return; > + prev = oidmap_put(map, entry); > + if (!check(prev != NULL)) > + return; > + check_str(prev->name, "one"); > + free(prev); > + > + FLEX_ALLOC_STR(entry, name, "deux"); > + if (get_oid_arbitrary_hex("22", &entry->entry.oid)) > + return; > + prev = oidmap_put(map, entry); > + if (!check(prev != NULL)) > + return; > + check_str(prev->name, "two"); > + free(prev); > +} > + > +static void t_get(struct oidmap *map) > +{ > + struct test_entry *entry; > + struct object_id oid; > + > + if (get_oid_arbitrary_hex("22", &oid)) > + return; > + entry = oidmap_get(map, &oid); > + if (!check(entry != NULL)) > + return; > + check_str(entry->name, "two"); > + > + if (get_oid_arbitrary_hex("44", &oid)) > + return; > + check(oidmap_get(map, &oid) == NULL); > + > + if (get_oid_arbitrary_hex("11", &oid)) > + return; > + entry = oidmap_get(map, &oid); > + if (!check(entry != NULL)) > + return; > + check_str(entry->name, "one"); > +} > + > +static void t_remove(struct oidmap *map) > +{ > + struct test_entry *entry; > + struct object_id oid; > + > + if (get_oid_arbitrary_hex("11", &oid)) > + return; > + entry = oidmap_remove(map, &oid); > + if (!check(entry != NULL)) > + return; > + check_str(entry->name, "one"); > + check(oidmap_get(map, &oid) == NULL); > + free(entry); > + > + if (get_oid_arbitrary_hex("22", &oid)) > + return; > + entry = oidmap_remove(map, &oid); > + if (!check(entry != NULL)) > + return; > + check_str(entry->name, "two"); > + check(oidmap_get(map, &oid) == NULL); > + free(entry); > + > + if (get_oid_arbitrary_hex("44", &oid)) > + return; > + check(oidmap_remove(map, &oid) == NULL); > +} > + > +static int key_val_contains(struct test_entry *entry) > +{ > + /* the test is small enough to be able to bear O(n) */ > + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > + if (!strcmp(key_val[i][1], entry->name)) { > + struct object_id oid; > + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) > + return -1; > + if (oideq(&entry->entry.oid, &oid)) > + return 0; > + } > + } > + return 1; > +} > + > +static void t_iterate(struct oidmap *map) > +{ > + struct oidmap_iter iter; > + struct test_entry *entry; > + int ret; > + > + oidmap_iter_init(map, &iter); > + while ((entry = oidmap_iter_next(&iter))) { > + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > + if (ret == -1) > + return; > + test_msg("obtained entry was not given in the input\n" > + " name: %s\n oid: %s\n", > + entry->name, oid_to_hex(&entry->entry.oid)); > + } > + } > + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); > +} > + > +int cmd_main(int argc UNUSED, const char **argv UNUSED) > +{ > + TEST(setup(t_replace), "replace works"); > + TEST(setup(t_get), "get works"); > + TEST(setup(t_remove), "remove works"); > + TEST(setup(t_iterate), "iterate works"); > + return test_done(); > +} > -- > 2.45.2 > >
Jonathan Nieder <jrnieder@gmail.com> wrote: > Ghanshyam Thakkar wrote: > > > helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h > > library which is built on top of hashmap.h to store arbitrary > > datastructure (which must contain oidmap_entry, which is a wrapper > > around object_id). These entries can be accessed by querying their > > associated object_id. > > > > Migrate them to the unit testing framework for better performance, > > concise code and better debugging. Along with the migration also plug > > memory leaks and make the test logic independent for all the tests. > > The migration removes 'put' tests from t0016, because it is used as > > setup to all the other tests, so testing it separately does not yield > > any benefit. > > > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > > --- > > This patch is depenedent on 'gt/unit-test-oidtree' for lib-oid. > > Very neat! I'm cc-ing Josh Steadmon for unit test framework expertise. > > Patch left unsnipped for reference. Friendly reminder for reviews/acks. :) Thanks. > > Makefile | 2 +- > > t/helper/test-oidmap.c | 123 ------------------------------ > > t/helper/test-tool.c | 1 - > > t/helper/test-tool.h | 1 - > > t/t0016-oidmap.sh | 112 --------------------------- > > t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++ > > 6 files changed, 166 insertions(+), 238 deletions(-) > > delete mode 100644 t/helper/test-oidmap.c > > delete mode 100755 t/t0016-oidmap.sh > > create mode 100644 t/unit-tests/t-oidmap.c > > > > diff --git a/Makefile b/Makefile > > index 03751e0fc0..f7ed50f3a9 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -810,7 +810,6 @@ 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 > > TEST_BUILTINS_OBJS += test-parse-options.o > > @@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/% > > > > UNIT_TEST_PROGRAMS += t-ctype > > UNIT_TEST_PROGRAMS += t-mem-pool > > +UNIT_TEST_PROGRAMS += t-oidmap > > UNIT_TEST_PROGRAMS += t-oidtree > > UNIT_TEST_PROGRAMS += t-prio-queue > > UNIT_TEST_PROGRAMS += t-strbuf > > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c > > deleted file mode 100644 > > index bd30244a54..0000000000 > > --- a/t/helper/test-oidmap.c > > +++ /dev/null > > @@ -1,123 +0,0 @@ > > -#include "test-tool.h" > > -#include "hex.h" > > -#include "object-name.h" > > -#include "oidmap.h" > > -#include "repository.h" > > -#include "setup.h" > > -#include "strbuf.h" > > -#include "string-list.h" > > - > > -/* key is an oid and value is a name (could be a refname for example) */ > > -struct test_entry { > > - struct oidmap_entry entry; > > - char name[FLEX_ARRAY]; > > -}; > > - > > -#define DELIM " \t\r\n" > > - > > -/* > > - * Read stdin line by line and print result of commands to stdout: > > - * > > - * hash oidkey -> sha1hash(oidkey) > > - * put oidkey namevalue -> NULL / old namevalue > > - * get oidkey -> NULL / namevalue > > - * remove oidkey -> NULL / old namevalue > > - * iterate -> oidkey1 namevalue1\noidkey2 namevalue2\n... > > - * > > - */ > > -int cmd__oidmap(int argc UNUSED, const char **argv UNUSED) > > -{ > > - struct string_list parts = STRING_LIST_INIT_NODUP; > > - struct strbuf line = STRBUF_INIT; > > - struct oidmap map = OIDMAP_INIT; > > - > > - setup_git_directory(); > > - > > - /* init oidmap */ > > - oidmap_init(&map, 0); > > - > > - /* process commands from stdin */ > > - while (strbuf_getline(&line, stdin) != EOF) { > > - char *cmd, *p1, *p2; > > - struct test_entry *entry; > > - struct object_id oid; > > - > > - /* break line into command and up to two parameters */ > > - string_list_setlen(&parts, 0); > > - string_list_split_in_place(&parts, line.buf, DELIM, 2); > > - string_list_remove_empty_items(&parts, 0); > > - > > - /* ignore empty lines */ > > - if (!parts.nr) > > - continue; > > - if (!*parts.items[0].string || *parts.items[0].string == '#') > > - continue; > > - > > - cmd = parts.items[0].string; > > - p1 = parts.nr >= 1 ? parts.items[1].string : NULL; > > - p2 = parts.nr >= 2 ? parts.items[2].string : NULL; > > - > > - if (!strcmp("put", cmd) && p1 && p2) { > > - > > - if (repo_get_oid(the_repository, p1, &oid)) { > > - printf("Unknown oid: %s\n", p1); > > - continue; > > - } > > - > > - /* create entry with oid_key = p1, name_value = p2 */ > > - FLEX_ALLOC_STR(entry, name, p2); > > - oidcpy(&entry->entry.oid, &oid); > > - > > - /* add / replace entry */ > > - entry = oidmap_put(&map, entry); > > - > > - /* print and free replaced entry, if any */ > > - puts(entry ? entry->name : "NULL"); > > - free(entry); > > - > > - } else if (!strcmp("get", cmd) && p1) { > > - > > - if (repo_get_oid(the_repository, p1, &oid)) { > > - printf("Unknown oid: %s\n", p1); > > - continue; > > - } > > - > > - /* lookup entry in oidmap */ > > - entry = oidmap_get(&map, &oid); > > - > > - /* print result */ > > - puts(entry ? entry->name : "NULL"); > > - > > - } else if (!strcmp("remove", cmd) && p1) { > > - > > - if (repo_get_oid(the_repository, p1, &oid)) { > > - printf("Unknown oid: %s\n", p1); > > - continue; > > - } > > - > > - /* remove entry from oidmap */ > > - entry = oidmap_remove(&map, &oid); > > - > > - /* print result and free entry*/ > > - puts(entry ? entry->name : "NULL"); > > - free(entry); > > - > > - } else if (!strcmp("iterate", cmd)) { > > - > > - struct oidmap_iter iter; > > - oidmap_iter_init(&map, &iter); > > - while ((entry = oidmap_iter_next(&iter))) > > - printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); > > - > > - } else { > > - > > - printf("Unknown command %s\n", cmd); > > - > > - } > > - } > > - > > - string_list_clear(&parts, 0); > > - strbuf_release(&line); > > - oidmap_free(&map, 1); > > - return 0; > > -} > > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > > index 253324a06b..5f013d8b2b 100644 > > --- a/t/helper/test-tool.c > > +++ b/t/helper/test-tool.c > > @@ -45,7 +45,6 @@ static struct test_cmd cmds[] = { > > { "mergesort", cmd__mergesort }, > > { "mktemp", cmd__mktemp }, > > { "oid-array", cmd__oid_array }, > > - { "oidmap", cmd__oidmap }, > > { "online-cpus", cmd__online_cpus }, > > { "pack-mtimes", cmd__pack_mtimes }, > > { "parse-options", cmd__parse_options }, > > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > > index 460dd7d260..c7d3e43694 100644 > > --- a/t/helper/test-tool.h > > +++ b/t/helper/test-tool.h > > @@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv); > > int cmd__match_trees(int argc, const char **argv); > > int cmd__mergesort(int argc, const char **argv); > > int cmd__mktemp(int argc, const char **argv); > > -int cmd__oidmap(int argc, const char **argv); > > int cmd__online_cpus(int argc, const char **argv); > > int cmd__pack_mtimes(int argc, const char **argv); > > int cmd__parse_options(int argc, const char **argv); > > diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh > > deleted file mode 100755 > > index 0faef1f4f1..0000000000 > > --- a/t/t0016-oidmap.sh > > +++ /dev/null > > @@ -1,112 +0,0 @@ > > -#!/bin/sh > > - > > -test_description='test oidmap' > > - > > -TEST_PASSES_SANITIZE_LEAK=true > > -. ./test-lib.sh > > - > > -# This purposefully is very similar to t0011-hashmap.sh > > - > > -test_oidmap () { > > - echo "$1" | test-tool oidmap $3 >actual && > > - echo "$2" >expect && > > - test_cmp expect actual > > -} > > - > > - > > -test_expect_success 'setup' ' > > - > > - test_commit one && > > - test_commit two && > > - test_commit three && > > - test_commit four > > - > > -' > > - > > -test_expect_success 'put' ' > > - > > -test_oidmap "put one 1 > > -put two 2 > > -put invalidOid 4 > > -put three 3" "NULL > > -NULL > > -Unknown oid: invalidOid > > -NULL" > > - > > -' > > - > > -test_expect_success 'replace' ' > > - > > -test_oidmap "put one 1 > > -put two 2 > > -put three 3 > > -put invalidOid 4 > > -put two deux > > -put one un" "NULL > > -NULL > > -NULL > > -Unknown oid: invalidOid > > -2 > > -1" > > - > > -' > > - > > -test_expect_success 'get' ' > > - > > -test_oidmap "put one 1 > > -put two 2 > > -put three 3 > > -get two > > -get four > > -get invalidOid > > -get one" "NULL > > -NULL > > -NULL > > -2 > > -NULL > > -Unknown oid: invalidOid > > -1" > > - > > -' > > - > > -test_expect_success 'remove' ' > > - > > -test_oidmap "put one 1 > > -put two 2 > > -put three 3 > > -remove one > > -remove two > > -remove invalidOid > > -remove four" "NULL > > -NULL > > -NULL > > -1 > > -2 > > -Unknown oid: invalidOid > > -NULL" > > - > > -' > > - > > -test_expect_success 'iterate' ' > > - test-tool oidmap >actual.raw <<-\EOF && > > - put one 1 > > - put two 2 > > - put three 3 > > - iterate > > - EOF > > - > > - # sort "expect" too so we do not rely on the order of particular oids > > - sort >expect <<-EOF && > > - NULL > > - NULL > > - NULL > > - $(git rev-parse one) 1 > > - $(git rev-parse two) 2 > > - $(git rev-parse three) 3 > > - EOF > > - > > - sort <actual.raw >actual && > > - test_cmp expect actual > > -' > > - > > -test_done > > diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c > > new file mode 100644 > > index 0000000000..9b98a3ed09 > > --- /dev/null > > +++ b/t/unit-tests/t-oidmap.c > > @@ -0,0 +1,165 @@ > > +#include "test-lib.h" > > +#include "lib-oid.h" > > +#include "oidmap.h" > > +#include "hash.h" > > +#include "hex.h" > > + > > +/* > > + * elements we will put in oidmap structs are made of a key: the entry.oid > > + * field, which is of type struct object_id, and a value: the name field (could > > + * be a refname for example) > > + */ > > +struct test_entry { > > + struct oidmap_entry entry; > > + char name[FLEX_ARRAY]; > > +}; > > + > > +static const char *key_val[][2] = { { "11", "one" }, > > + { "22", "two" }, > > + { "33", "three" } }; > > + > > +static int put_and_check_null(struct oidmap *map, const char *hex, > > + const char *entry_name) > > +{ > > + struct test_entry *entry; > > + > > + FLEX_ALLOC_STR(entry, name, entry_name); > > + if (get_oid_arbitrary_hex(hex, &entry->entry.oid)) > > + return -1; > > + if (!check(oidmap_put(map, entry) == NULL)) > > + return -1; > > + return 0; > > +} > > + > > +static void setup(void (*f)(struct oidmap *map)) > > +{ > > + struct oidmap map = OIDMAP_INIT; > > + int ret = 0; > > + > > + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) > > + if ((ret = put_and_check_null(&map, key_val[i][0], > > + key_val[i][1]))) > > + break; > > + > > + if (!ret) > > + f(&map); > > + oidmap_free(&map, 1); > > +} > > + > > +static void t_replace(struct oidmap *map) > > +{ > > + struct test_entry *entry, *prev; > > + > > + FLEX_ALLOC_STR(entry, name, "un"); > > + if (get_oid_arbitrary_hex("11", &entry->entry.oid)) > > + return; > > + prev = oidmap_put(map, entry); > > + if (!check(prev != NULL)) > > + return; > > + check_str(prev->name, "one"); > > + free(prev); > > + > > + FLEX_ALLOC_STR(entry, name, "deux"); > > + if (get_oid_arbitrary_hex("22", &entry->entry.oid)) > > + return; > > + prev = oidmap_put(map, entry); > > + if (!check(prev != NULL)) > > + return; > > + check_str(prev->name, "two"); > > + free(prev); > > +} > > + > > +static void t_get(struct oidmap *map) > > +{ > > + struct test_entry *entry; > > + struct object_id oid; > > + > > + if (get_oid_arbitrary_hex("22", &oid)) > > + return; > > + entry = oidmap_get(map, &oid); > > + if (!check(entry != NULL)) > > + return; > > + check_str(entry->name, "two"); > > + > > + if (get_oid_arbitrary_hex("44", &oid)) > > + return; > > + check(oidmap_get(map, &oid) == NULL); > > + > > + if (get_oid_arbitrary_hex("11", &oid)) > > + return; > > + entry = oidmap_get(map, &oid); > > + if (!check(entry != NULL)) > > + return; > > + check_str(entry->name, "one"); > > +} > > + > > +static void t_remove(struct oidmap *map) > > +{ > > + struct test_entry *entry; > > + struct object_id oid; > > + > > + if (get_oid_arbitrary_hex("11", &oid)) > > + return; > > + entry = oidmap_remove(map, &oid); > > + if (!check(entry != NULL)) > > + return; > > + check_str(entry->name, "one"); > > + check(oidmap_get(map, &oid) == NULL); > > + free(entry); > > + > > + if (get_oid_arbitrary_hex("22", &oid)) > > + return; > > + entry = oidmap_remove(map, &oid); > > + if (!check(entry != NULL)) > > + return; > > + check_str(entry->name, "two"); > > + check(oidmap_get(map, &oid) == NULL); > > + free(entry); > > + > > + if (get_oid_arbitrary_hex("44", &oid)) > > + return; > > + check(oidmap_remove(map, &oid) == NULL); > > +} > > + > > +static int key_val_contains(struct test_entry *entry) > > +{ > > + /* the test is small enough to be able to bear O(n) */ > > + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > > + if (!strcmp(key_val[i][1], entry->name)) { > > + struct object_id oid; > > + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) > > + return -1; > > + if (oideq(&entry->entry.oid, &oid)) > > + return 0; > > + } > > + } > > + return 1; > > +} > > + > > +static void t_iterate(struct oidmap *map) > > +{ > > + struct oidmap_iter iter; > > + struct test_entry *entry; > > + int ret; > > + > > + oidmap_iter_init(map, &iter); > > + while ((entry = oidmap_iter_next(&iter))) { > > + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > > + if (ret == -1) > > + return; > > + test_msg("obtained entry was not given in the input\n" > > + " name: %s\n oid: %s\n", > > + entry->name, oid_to_hex(&entry->entry.oid)); > > + } > > + } > > + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); > > +} > > + > > +int cmd_main(int argc UNUSED, const char **argv UNUSED) > > +{ > > + TEST(setup(t_replace), "replace works"); > > + TEST(setup(t_get), "get works"); > > + TEST(setup(t_remove), "remove works"); > > + TEST(setup(t_iterate), "iterate works"); > > + return test_done(); > > +} > > -- > > 2.45.2 > > > >
Hi Ghanshyam On 20/06/2024 10:45, Jonathan Nieder wrote: > Ghanshyam Thakkar wrote: > >> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h >> library which is built on top of hashmap.h to store arbitrary >> datastructure (which must contain oidmap_entry, which is a wrapper >> around object_id). I'm not really sure what the sentence is trying to say. I think it would be helpful to start the commit message with an introductory sentence explaining that the oidmap is currently tested via `test-tool` and this commit converts those tests to unit tests. These entries can be accessed by querying their >> associated object_id. >> >> Migrate them to the unit testing framework for better performance, >> concise code and better debugging. Along with the migration also plug >> memory leaks and make the test logic independent for all the tests. >> The migration removes 'put' tests from t0016, because it is used as >> setup to all the other tests, so testing it separately does not yield >> any benefit. Thanks sounds sensible, thanks for explaining it in the commit message. Overall the patch looks pretty good, I've left a couple of comments below. >> Mentored-by: Christian Couder <chriscool@tuxfamily.org> >> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> >> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> >> --- >> diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c >> new file mode 100644 >> index 0000000000..9b98a3ed09 >> --- /dev/null >> +++ b/t/unit-tests/t-oidmap.c >> @@ -0,0 +1,165 @@ >> +#include "test-lib.h" >> +#include "lib-oid.h" >> +#include "oidmap.h" >> +#include "hash.h" >> +#include "hex.h" >> + >> +/* >> + * elements we will put in oidmap structs are made of a key: the entry.oid >> + * field, which is of type struct object_id, and a value: the name field (could >> + * be a refname for example) >> + */ >> +struct test_entry { >> + struct oidmap_entry entry; >> + char name[FLEX_ARRAY]; >> +}; >> + >> +static const char *key_val[][2] = { { "11", "one" }, >> + { "22", "two" }, >> + { "33", "three" } }; >> + >> +static int put_and_check_null(struct oidmap *map, const char *hex, >> + const char *entry_name) >> +{ >> + struct test_entry *entry; >> + >> + FLEX_ALLOC_STR(entry, name, entry_name); >> + if (get_oid_arbitrary_hex(hex, &entry->entry.oid)) >> + return -1; When writing unit tests it is important to make sure that they fail, rather than just return early if there is an error. There are a number of places like this that return early without calling one of the check() macros to make the test fail. >> + if (!check(oidmap_put(map, entry) == NULL)) >> + return -1; >> + return 0; >> +} >> + >> +static void setup(void (*f)(struct oidmap *map)) >> +{ >> + struct oidmap map = OIDMAP_INIT; >> + int ret = 0; >> + >> + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) >> + if ((ret = put_and_check_null(&map, key_val[i][0], >> + key_val[i][1]))) Given there is only one caller I think it would be easier to see what is going on if the function body was just inlined into the loop here. >> + break; >> + >> + if (!ret) >> + f(&map); >> + oidmap_free(&map, 1); >> +} The tests for replace, get, remove all look like faithful translations of the old script and are fine apart from some missing check() calls when get_oid_arbitrary_hex() fails. >> +static int key_val_contains(struct test_entry *entry) >> +{ >> + /* the test is small enough to be able to bear O(n) */ It is good to think about that but I'm not sure we need a comment about it in a small test like this. >> + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { >> + if (!strcmp(key_val[i][1], entry->name)) { >> + struct object_id oid; >> + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) >> + return -1; >> + if (oideq(&entry->entry.oid, &oid)) >> + return 0; >> + } >> + } >> + return 1; >> +} So if we cannot construct the oid we return -1, if the oid matches we return 0 and if the oid does not match we return 1 >> +static void t_iterate(struct oidmap *map) >> +{ >> + struct oidmap_iter iter; >> + struct test_entry *entry; >> + int ret; >> + >> + oidmap_iter_init(map, &iter); >> + while ((entry = oidmap_iter_next(&iter))) { >> + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { >> + if (ret == -1) >> + return; >> + test_msg("obtained entry was not given in the input\n" >> + " name: %s\n oid: %s\n", >> + entry->name, oid_to_hex(&entry->entry.oid)); This checks that all of the expect objects are present, but does not check for duplicate objects. An alternative would be to build an array of all the entries, then sort it by oid and compare that to a sorted version of `key_val`. That is what the scripted version does. We don't have any helpers for comparing arrays so you'd need to do that by comparing each element in a loop. >> + } >> + } >> + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); One could argue that this helps guard against duplicate entries but that's only true if we trust hashmap_get_size() so I think keeping this to check that hashmap_get_size() gives the correct size and changing the loop above would be better. Thanks Phillip >> +} >> + >> +int cmd_main(int argc UNUSED, const char **argv UNUSED) >> +{ >> + TEST(setup(t_replace), "replace works"); >> + TEST(setup(t_get), "get works"); >> + TEST(setup(t_remove), "remove works"); >> + TEST(setup(t_iterate), "iterate works"); >> + return test_done(); >> +} >> -- >> 2.45.2 >> >> >
Phillip Wood <phillip.wood123@gmail.com> wrote: > Hi Ghanshyam > > On 20/06/2024 10:45, Jonathan Nieder wrote: > > Ghanshyam Thakkar wrote: > > > >> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h > >> library which is built on top of hashmap.h to store arbitrary > >> datastructure (which must contain oidmap_entry, which is a wrapper > >> around object_id). > > I'm not really sure what the sentence is trying to say. I think it would > be helpful to start the commit message with an introductory sentence > explaining that the oidmap is currently tested via `test-tool` and this > commit converts those tests to unit tests. Got it. Will improve. I just wanted to explain the basics of oidmap to help ease the review process. > These entries can be accessed by querying their > >> associated object_id. > >> > >> Migrate them to the unit testing framework for better performance, > >> concise code and better debugging. Along with the migration also plug > >> memory leaks and make the test logic independent for all the tests. > > >> The migration removes 'put' tests from t0016, because it is used as > >> setup to all the other tests, so testing it separately does not yield > >> any benefit. > > Thanks sounds sensible, thanks for explaining it in the commit message. > > Overall the patch looks pretty good, I've left a couple of comments > below. > > >> Mentored-by: Christian Couder <chriscool@tuxfamily.org> > >> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > >> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > >> --- > > >> diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c > >> new file mode 100644 > >> index 0000000000..9b98a3ed09 > >> --- /dev/null > >> +++ b/t/unit-tests/t-oidmap.c > >> @@ -0,0 +1,165 @@ > >> +#include "test-lib.h" > >> +#include "lib-oid.h" > >> +#include "oidmap.h" > >> +#include "hash.h" > >> +#include "hex.h" > >> + > >> +/* > >> + * elements we will put in oidmap structs are made of a key: the entry.oid > >> + * field, which is of type struct object_id, and a value: the name field (could > >> + * be a refname for example) > >> + */ > >> +struct test_entry { > >> + struct oidmap_entry entry; > >> + char name[FLEX_ARRAY]; > >> +}; > >> + > >> +static const char *key_val[][2] = { { "11", "one" }, > >> + { "22", "two" }, > >> + { "33", "three" } }; > >> + > >> +static int put_and_check_null(struct oidmap *map, const char *hex, > >> + const char *entry_name) > >> +{ > >> + struct test_entry *entry; > >> + > >> + FLEX_ALLOC_STR(entry, name, entry_name); > >> + if (get_oid_arbitrary_hex(hex, &entry->entry.oid)) > >> + return -1; > > When writing unit tests it is important to make sure that they fail, > rather than just return early if there is an error. There are a number > of places like this that return early without calling one of the check() > macros to make the test fail. They do fail. `get_oid_arbitrary_hex()` from 'unit-tests/lib-oid.h' is a function specifically built for the use in unit tests. And it contains in built `check_*` to ensure that the tests fails if something goes wrong and also prints diagnostic info. Maybe we can add a check here as well to know the line number at which the call failed, but since we already print queried hex value and other diagnostic info from `get_oid_arbitrary_hex()`, I thought it would be enough. > >> + if (!check(oidmap_put(map, entry) == NULL)) > >> + return -1; > >> + return 0; > >> +} > >> + > >> +static void setup(void (*f)(struct oidmap *map)) > >> +{ > >> + struct oidmap map = OIDMAP_INIT; > >> + int ret = 0; > >> + > >> + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) > >> + if ((ret = put_and_check_null(&map, key_val[i][0], > >> + key_val[i][1]))) > > Given there is only one caller I think it would be easier to see what is > going on if the function body was just inlined into the loop here. Yeah, will do. > >> + break; > >> + > >> + if (!ret) > >> + f(&map); > >> + oidmap_free(&map, 1); > >> +} > > The tests for replace, get, remove all look like faithful translations > of the old script and are fine apart from some missing check() calls > when get_oid_arbitrary_hex() fails. > > >> +static int key_val_contains(struct test_entry *entry) > >> +{ > >> + /* the test is small enough to be able to bear O(n) */ > > It is good to think about that but I'm not sure we need a comment about > it in a small test like this. Got it. Will remove. > >> + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > >> + if (!strcmp(key_val[i][1], entry->name)) { > >> + struct object_id oid; > >> + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) > >> + return -1; > >> + if (oideq(&entry->entry.oid, &oid)) > >> + return 0; > >> + } > >> + } > >> + return 1; > >> +} > > So if we cannot construct the oid we return -1, if the oid matches we > return 0 and if the oid does not match we return 1 > > >> +static void t_iterate(struct oidmap *map) > >> +{ > >> + struct oidmap_iter iter; > >> + struct test_entry *entry; > >> + int ret; > >> + > >> + oidmap_iter_init(map, &iter); > >> + while ((entry = oidmap_iter_next(&iter))) { > >> + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > >> + if (ret == -1) > >> + return; > >> + test_msg("obtained entry was not given in the input\n" > >> + " name: %s\n oid: %s\n", > >> + entry->name, oid_to_hex(&entry->entry.oid)); > > This checks that all of the expect objects are present, but does not > check for duplicate objects. An alternative would be to build an array > of all the entries, then sort it by oid and compare that to a sorted > version of `key_val`. That is what the scripted version does. We don't > have any helpers for comparing arrays so you'd need to do that by > comparing each element in a loop. > > >> + } > >> + } > >> + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); > > One could argue that this helps guard against duplicate entries but > that's only true if we trust hashmap_get_size() so I think keeping this > to check that hashmap_get_size() gives the correct size and changing the > loop above would be better. Yeah, since I was not sure if hashmap's order is predictable, I first checked if the entry exists and later checked if the size matches. I'll try to do the array approach you mentioned. Thank you for the review.
Hi Ghanshyam On 25/06/2024 20:16, Ghanshyam Thakkar wrote: > Phillip Wood <phillip.wood123@gmail.com> wrote: >> When writing unit tests it is important to make sure that they fail, >> rather than just return early if there is an error. There are a number >> of places like this that return early without calling one of the check() >> macros to make the test fail. > > They do fail. `get_oid_arbitrary_hex()` from 'unit-tests/lib-oid.h' is > a function specifically built for the use in unit tests. And it > contains in built `check_*` to ensure that the tests fails if something > goes wrong and also prints diagnostic info. Maybe we can add a check here > as well to know the line number at which the call failed, but since we > already print queried hex value and other diagnostic info from > `get_oid_arbitrary_hex()`, I thought it would be enough. Oh, sorry I didn't realize that. I agree that the check in get_oid_arbitary_hex() should be sufficient. Best Wishes Phillip
diff --git a/Makefile b/Makefile index 03751e0fc0..f7ed50f3a9 100644 --- a/Makefile +++ b/Makefile @@ -810,7 +810,6 @@ 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 TEST_BUILTINS_OBJS += test-parse-options.o @@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-mem-pool +UNIT_TEST_PROGRAMS += t-oidmap UNIT_TEST_PROGRAMS += t-oidtree UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-strbuf diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c deleted file mode 100644 index bd30244a54..0000000000 --- a/t/helper/test-oidmap.c +++ /dev/null @@ -1,123 +0,0 @@ -#include "test-tool.h" -#include "hex.h" -#include "object-name.h" -#include "oidmap.h" -#include "repository.h" -#include "setup.h" -#include "strbuf.h" -#include "string-list.h" - -/* key is an oid and value is a name (could be a refname for example) */ -struct test_entry { - struct oidmap_entry entry; - char name[FLEX_ARRAY]; -}; - -#define DELIM " \t\r\n" - -/* - * Read stdin line by line and print result of commands to stdout: - * - * hash oidkey -> sha1hash(oidkey) - * put oidkey namevalue -> NULL / old namevalue - * get oidkey -> NULL / namevalue - * remove oidkey -> NULL / old namevalue - * iterate -> oidkey1 namevalue1\noidkey2 namevalue2\n... - * - */ -int cmd__oidmap(int argc UNUSED, const char **argv UNUSED) -{ - struct string_list parts = STRING_LIST_INIT_NODUP; - struct strbuf line = STRBUF_INIT; - struct oidmap map = OIDMAP_INIT; - - setup_git_directory(); - - /* init oidmap */ - oidmap_init(&map, 0); - - /* process commands from stdin */ - while (strbuf_getline(&line, stdin) != EOF) { - char *cmd, *p1, *p2; - struct test_entry *entry; - struct object_id oid; - - /* break line into command and up to two parameters */ - string_list_setlen(&parts, 0); - string_list_split_in_place(&parts, line.buf, DELIM, 2); - string_list_remove_empty_items(&parts, 0); - - /* ignore empty lines */ - if (!parts.nr) - continue; - if (!*parts.items[0].string || *parts.items[0].string == '#') - continue; - - cmd = parts.items[0].string; - p1 = parts.nr >= 1 ? parts.items[1].string : NULL; - p2 = parts.nr >= 2 ? parts.items[2].string : NULL; - - if (!strcmp("put", cmd) && p1 && p2) { - - if (repo_get_oid(the_repository, p1, &oid)) { - printf("Unknown oid: %s\n", p1); - continue; - } - - /* create entry with oid_key = p1, name_value = p2 */ - FLEX_ALLOC_STR(entry, name, p2); - oidcpy(&entry->entry.oid, &oid); - - /* add / replace entry */ - entry = oidmap_put(&map, entry); - - /* print and free replaced entry, if any */ - puts(entry ? entry->name : "NULL"); - free(entry); - - } else if (!strcmp("get", cmd) && p1) { - - if (repo_get_oid(the_repository, p1, &oid)) { - printf("Unknown oid: %s\n", p1); - continue; - } - - /* lookup entry in oidmap */ - entry = oidmap_get(&map, &oid); - - /* print result */ - puts(entry ? entry->name : "NULL"); - - } else if (!strcmp("remove", cmd) && p1) { - - if (repo_get_oid(the_repository, p1, &oid)) { - printf("Unknown oid: %s\n", p1); - continue; - } - - /* remove entry from oidmap */ - entry = oidmap_remove(&map, &oid); - - /* print result and free entry*/ - puts(entry ? entry->name : "NULL"); - free(entry); - - } else if (!strcmp("iterate", cmd)) { - - struct oidmap_iter iter; - oidmap_iter_init(&map, &iter); - while ((entry = oidmap_iter_next(&iter))) - printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); - - } else { - - printf("Unknown command %s\n", cmd); - - } - } - - string_list_clear(&parts, 0); - strbuf_release(&line); - oidmap_free(&map, 1); - return 0; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 253324a06b..5f013d8b2b 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -45,7 +45,6 @@ static struct test_cmd cmds[] = { { "mergesort", cmd__mergesort }, { "mktemp", cmd__mktemp }, { "oid-array", cmd__oid_array }, - { "oidmap", cmd__oidmap }, { "online-cpus", cmd__online_cpus }, { "pack-mtimes", cmd__pack_mtimes }, { "parse-options", cmd__parse_options }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 460dd7d260..c7d3e43694 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv); int cmd__match_trees(int argc, const char **argv); int cmd__mergesort(int argc, const char **argv); int cmd__mktemp(int argc, const char **argv); -int cmd__oidmap(int argc, const char **argv); int cmd__online_cpus(int argc, const char **argv); int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh deleted file mode 100755 index 0faef1f4f1..0000000000 --- a/t/t0016-oidmap.sh +++ /dev/null @@ -1,112 +0,0 @@ -#!/bin/sh - -test_description='test oidmap' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -# This purposefully is very similar to t0011-hashmap.sh - -test_oidmap () { - echo "$1" | test-tool oidmap $3 >actual && - echo "$2" >expect && - test_cmp expect actual -} - - -test_expect_success 'setup' ' - - test_commit one && - test_commit two && - test_commit three && - test_commit four - -' - -test_expect_success 'put' ' - -test_oidmap "put one 1 -put two 2 -put invalidOid 4 -put three 3" "NULL -NULL -Unknown oid: invalidOid -NULL" - -' - -test_expect_success 'replace' ' - -test_oidmap "put one 1 -put two 2 -put three 3 -put invalidOid 4 -put two deux -put one un" "NULL -NULL -NULL -Unknown oid: invalidOid -2 -1" - -' - -test_expect_success 'get' ' - -test_oidmap "put one 1 -put two 2 -put three 3 -get two -get four -get invalidOid -get one" "NULL -NULL -NULL -2 -NULL -Unknown oid: invalidOid -1" - -' - -test_expect_success 'remove' ' - -test_oidmap "put one 1 -put two 2 -put three 3 -remove one -remove two -remove invalidOid -remove four" "NULL -NULL -NULL -1 -2 -Unknown oid: invalidOid -NULL" - -' - -test_expect_success 'iterate' ' - test-tool oidmap >actual.raw <<-\EOF && - put one 1 - put two 2 - put three 3 - iterate - EOF - - # sort "expect" too so we do not rely on the order of particular oids - sort >expect <<-EOF && - NULL - NULL - NULL - $(git rev-parse one) 1 - $(git rev-parse two) 2 - $(git rev-parse three) 3 - EOF - - sort <actual.raw >actual && - test_cmp expect actual -' - -test_done diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c new file mode 100644 index 0000000000..9b98a3ed09 --- /dev/null +++ b/t/unit-tests/t-oidmap.c @@ -0,0 +1,165 @@ +#include "test-lib.h" +#include "lib-oid.h" +#include "oidmap.h" +#include "hash.h" +#include "hex.h" + +/* + * elements we will put in oidmap structs are made of a key: the entry.oid + * field, which is of type struct object_id, and a value: the name field (could + * be a refname for example) + */ +struct test_entry { + struct oidmap_entry entry; + char name[FLEX_ARRAY]; +}; + +static const char *key_val[][2] = { { "11", "one" }, + { "22", "two" }, + { "33", "three" } }; + +static int put_and_check_null(struct oidmap *map, const char *hex, + const char *entry_name) +{ + struct test_entry *entry; + + FLEX_ALLOC_STR(entry, name, entry_name); + if (get_oid_arbitrary_hex(hex, &entry->entry.oid)) + return -1; + if (!check(oidmap_put(map, entry) == NULL)) + return -1; + return 0; +} + +static void setup(void (*f)(struct oidmap *map)) +{ + struct oidmap map = OIDMAP_INIT; + int ret = 0; + + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) + if ((ret = put_and_check_null(&map, key_val[i][0], + key_val[i][1]))) + break; + + if (!ret) + f(&map); + oidmap_free(&map, 1); +} + +static void t_replace(struct oidmap *map) +{ + struct test_entry *entry, *prev; + + FLEX_ALLOC_STR(entry, name, "un"); + if (get_oid_arbitrary_hex("11", &entry->entry.oid)) + return; + prev = oidmap_put(map, entry); + if (!check(prev != NULL)) + return; + check_str(prev->name, "one"); + free(prev); + + FLEX_ALLOC_STR(entry, name, "deux"); + if (get_oid_arbitrary_hex("22", &entry->entry.oid)) + return; + prev = oidmap_put(map, entry); + if (!check(prev != NULL)) + return; + check_str(prev->name, "two"); + free(prev); +} + +static void t_get(struct oidmap *map) +{ + struct test_entry *entry; + struct object_id oid; + + if (get_oid_arbitrary_hex("22", &oid)) + return; + entry = oidmap_get(map, &oid); + if (!check(entry != NULL)) + return; + check_str(entry->name, "two"); + + if (get_oid_arbitrary_hex("44", &oid)) + return; + check(oidmap_get(map, &oid) == NULL); + + if (get_oid_arbitrary_hex("11", &oid)) + return; + entry = oidmap_get(map, &oid); + if (!check(entry != NULL)) + return; + check_str(entry->name, "one"); +} + +static void t_remove(struct oidmap *map) +{ + struct test_entry *entry; + struct object_id oid; + + if (get_oid_arbitrary_hex("11", &oid)) + return; + entry = oidmap_remove(map, &oid); + if (!check(entry != NULL)) + return; + check_str(entry->name, "one"); + check(oidmap_get(map, &oid) == NULL); + free(entry); + + if (get_oid_arbitrary_hex("22", &oid)) + return; + entry = oidmap_remove(map, &oid); + if (!check(entry != NULL)) + return; + check_str(entry->name, "two"); + check(oidmap_get(map, &oid) == NULL); + free(entry); + + if (get_oid_arbitrary_hex("44", &oid)) + return; + check(oidmap_remove(map, &oid) == NULL); +} + +static int key_val_contains(struct test_entry *entry) +{ + /* the test is small enough to be able to bear O(n) */ + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { + if (!strcmp(key_val[i][1], entry->name)) { + struct object_id oid; + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) + return -1; + if (oideq(&entry->entry.oid, &oid)) + return 0; + } + } + return 1; +} + +static void t_iterate(struct oidmap *map) +{ + struct oidmap_iter iter; + struct test_entry *entry; + int ret; + + oidmap_iter_init(map, &iter); + while ((entry = oidmap_iter_next(&iter))) { + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { + if (ret == -1) + return; + test_msg("obtained entry was not given in the input\n" + " name: %s\n oid: %s\n", + entry->name, oid_to_hex(&entry->entry.oid)); + } + } + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); +} + +int cmd_main(int argc UNUSED, const char **argv UNUSED) +{ + TEST(setup(t_replace), "replace works"); + TEST(setup(t_get), "get works"); + TEST(setup(t_remove), "remove works"); + TEST(setup(t_iterate), "iterate works"); + return test_done(); +}
helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h library which is built on top of hashmap.h to store arbitrary datastructure (which must contain oidmap_entry, which is a wrapper around object_id). These entries can be accessed by querying their associated object_id. Migrate them to the unit testing framework for better performance, concise code and better debugging. Along with the migration also plug memory leaks and make the test logic independent for all the tests. The migration removes 'put' tests from t0016, because it is used as setup to all the other tests, so testing it separately does not yield any benefit. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- This patch is depenedent on 'gt/unit-test-oidtree' for lib-oid. Makefile | 2 +- t/helper/test-oidmap.c | 123 ------------------------------ t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0016-oidmap.sh | 112 --------------------------- t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 166 insertions(+), 238 deletions(-) delete mode 100644 t/helper/test-oidmap.c delete mode 100755 t/t0016-oidmap.sh create mode 100644 t/unit-tests/t-oidmap.c