| Message ID | 20240628122030.41554-1-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [GSoC,v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c | expand |
On 2024.06.28 17:50, 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. > > 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. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > This version addresses Phillip's review about detecting duplicates in > oidmap when iterating over it and removing put_and_check_null() to move > the relevant code to setup() instead. And contains some grammer fixes > in the comment. IIUC this corrects all of the issues that Phillip noted in his earlier review, except for checking for duplicates, is that right? Personally I think this version is OK even without that check, and I'll be away from email for the rest of this week, so I'll go ahead and sign off: Reviewed-by: Josh Steadmon <steadmon@google.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h > library which is built on top of hashmap.h. > > 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. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > This version addresses Phillip's review about detecting duplicates in > oidmap when iterating over it ... Hmph. You seem to overwrite key_val[i][1] ... > +/* > + * 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" } }; ... in this test, rendering the key_val[] array unusuable for further tests. Is that intended and desirable? As long as t_iterate() stays to be the last test, that would be OK, but once somebody wants to add a new test after it, i.e. TEST(setup(t_iterate), "iterate works"); + TEST(setup(t_frotz), "frotz works"); return test_done(); the setup() for that new test depends on the key_val[] array, whose value fields key_val[i][1] have been modified. The TEST(setup(t_foo)) pattern is done so nicely to make sure that everybody is independent from everybody else, preparing the oidmap used for each specific test from scratch. It is a bit disappointing that we are now invalidating this nice property. > +static int key_val_contains(struct test_entry *entry) > +{ > + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > + struct object_id oid; > + > + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) > + return -1; > + > + if (oideq(&entry->entry.oid, &oid)) { > + if (!strcmp(key_val[i][1], "USED")) > + return 2; > + key_val[i][1] = "USED"; > + return 0; > + } > + } > + return 1; > +} Otherwise, the other tests looked reasonable. Looking really nice. As I expect things to be slow this week, being a big vacation week in the US, we may not see much review activities this week. I'll queue this version in the meantime, not merging it down to 'next', in case people start comment on it next week. Thanks. > +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(); > +}
Josh Steadmon <steadmon@google.com> writes: >> This version addresses Phillip's review about detecting duplicates in >> oidmap when iterating over it and removing put_and_check_null() to move >> the relevant code to setup() instead. And contains some grammer fixes >> in the comment. > > IIUC this corrects all of the issues that Phillip noted in his earlier > review, except for checking for duplicates, is that right? There is an attempted duplicate checking during iteration; the test data source key_val[] array is (ab)used to record the already seen keys during the iteration, which would work but is a hacky and unmaintainable way to do so. Thanks for reviewing.
Junio C Hamano <gitster@pobox.com> writes: > Hmph. You seem to overwrite key_val[i][1] ... > ... > ... in this test, rendering the key_val[] array unusuable for > further tests. Is that intended and desirable? > ... > The TEST(setup(t_foo)) pattern is done so nicely to make sure that > everybody is independent from everybody else, preparing the oidmap > used for each specific test from scratch. It is a bit disappointing > that we are now invalidating this nice property. It may be just the matter of doing something silly like this to restore the "different tests are independent and the source of truth array is intact" property. The first hunk should be reindented properly, if you are going to take this and squash into your patch, by the way. Thanks. t/unit-tests/t-oidmap.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c index 13532aa98b..be2741c6c7 100644 --- c/t/unit-tests/t-oidmap.c +++ w/t/unit-tests/t-oidmap.c @@ -14,7 +14,7 @@ struct test_entry { char name[FLEX_ARRAY]; }; -static const char *key_val[][2] = { { "11", "one" }, +static const char * const key_val[][2] = { { "11", "one" }, { "22", "two" }, { "33", "three" } }; @@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map) check(oidmap_remove(map, &oid) == NULL); } -static int key_val_contains(struct test_entry *entry) +static int key_val_contains(struct test_entry *entry, char seen[]) { for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { struct object_id oid; @@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry *entry) return -1; if (oideq(&entry->entry.oid, &oid)) { - if (!strcmp(key_val[i][1], "USED")) + if (seen[i]) return 2; - key_val[i][1] = "USED"; + seen[i] = 1; return 0; } } @@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map) { struct oidmap_iter iter; struct test_entry *entry; + char seen[ARRAY_SIZE(key_val)] = { 0 }; oidmap_iter_init(map, &iter); while ((entry = oidmap_iter_next(&iter))) { int ret; - if (!check_int((ret = key_val_contains(entry)), ==, 0)) { + if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) { switch (ret) { case -1: break; /* error message handled by get_oid_arbitrary_hex() */
Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Hmph. You seem to overwrite key_val[i][1] ... > > ... > > ... in this test, rendering the key_val[] array unusuable for > > further tests. Is that intended and desirable? > > ... > > The TEST(setup(t_foo)) pattern is done so nicely to make sure that > > everybody is independent from everybody else, preparing the oidmap > > used for each specific test from scratch. It is a bit disappointing > > that we are now invalidating this nice property. > > It may be just the matter of doing something silly like this to > restore the "different tests are independent and the source of truth > array is intact" property. > > The first hunk should be reindented properly, if you are going to > take this and squash into your patch, by the way. > > Thanks. I think this is very reasonable. I'll squash this into my patch and wait for any other comments from reviewers before sending another version. Thanks. > t/unit-tests/t-oidmap.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c > index 13532aa98b..be2741c6c7 100644 > --- c/t/unit-tests/t-oidmap.c > +++ w/t/unit-tests/t-oidmap.c > @@ -14,7 +14,7 @@ struct test_entry { > char name[FLEX_ARRAY]; > }; > > -static const char *key_val[][2] = { { "11", "one" }, > +static const char * const key_val[][2] = { { "11", "one" }, > { "22", "two" }, > { "33", "three" } }; > > @@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map) > check(oidmap_remove(map, &oid) == NULL); > } > > -static int key_val_contains(struct test_entry *entry) > +static int key_val_contains(struct test_entry *entry, char seen[]) > { > for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > struct object_id oid; > @@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry > *entry) > return -1; > > if (oideq(&entry->entry.oid, &oid)) { > - if (!strcmp(key_val[i][1], "USED")) > + if (seen[i]) > return 2; > - key_val[i][1] = "USED"; > + seen[i] = 1; > return 0; > } > } > @@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map) > { > struct oidmap_iter iter; > struct test_entry *entry; > + char seen[ARRAY_SIZE(key_val)] = { 0 }; > > oidmap_iter_init(map, &iter); > while ((entry = oidmap_iter_next(&iter))) { > int ret; > - if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > + if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) { > switch (ret) { > case -1: > break; /* error message handled by get_oid_arbitrary_hex() */
Hi Junio On 01/07/2024 23:20, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Hmph. You seem to overwrite key_val[i][1] ... >> ... >> ... in this test, rendering the key_val[] array unusuable for >> further tests. Is that intended and desirable? >> ... >> The TEST(setup(t_foo)) pattern is done so nicely to make sure that >> everybody is independent from everybody else, preparing the oidmap >> used for each specific test from scratch. It is a bit disappointing >> that we are now invalidating this nice property. > > It may be just the matter of doing something silly like this to > restore the "different tests are independent and the source of truth > array is intact" property. > > The first hunk should be reindented properly, if you are going to > take this and squash into your patch, by the way. This looks good - we should definitely avoid overwriting key_val. Best Wishes Phillip > Thanks. > > t/unit-tests/t-oidmap.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c > index 13532aa98b..be2741c6c7 100644 > --- c/t/unit-tests/t-oidmap.c > +++ w/t/unit-tests/t-oidmap.c > @@ -14,7 +14,7 @@ struct test_entry { > char name[FLEX_ARRAY]; > }; > > -static const char *key_val[][2] = { { "11", "one" }, > +static const char * const key_val[][2] = { { "11", "one" }, > { "22", "two" }, > { "33", "three" } }; > > @@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map) > check(oidmap_remove(map, &oid) == NULL); > } > > -static int key_val_contains(struct test_entry *entry) > +static int key_val_contains(struct test_entry *entry, char seen[]) > { > for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { > struct object_id oid; > @@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry *entry) > return -1; > > if (oideq(&entry->entry.oid, &oid)) { > - if (!strcmp(key_val[i][1], "USED")) > + if (seen[i]) > return 2; > - key_val[i][1] = "USED"; > + seen[i] = 1; > return 0; > } > } > @@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map) > { > struct oidmap_iter iter; > struct test_entry *entry; > + char seen[ARRAY_SIZE(key_val)] = { 0 }; > > oidmap_iter_init(map, &iter); > while ((entry = oidmap_iter_next(&iter))) { > int ret; > - if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > + if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) { > switch (ret) { > case -1: > break; /* error message handled by get_oid_arbitrary_hex() */ > > >
Hi Ghanshyam On 28/06/2024 13:20, 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. > > 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. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > This version addresses Phillip's review about detecting duplicates in > oidmap when iterating over it and removing put_and_check_null() to move > the relevant code to setup() instead. And contains some grammer fixes > in the comment. This version with Junio's fixup addresses my previous comments. One more thing occurred to me as I was reading it again > +static void t_iterate(struct oidmap *map) > +{ > + struct oidmap_iter iter; > + struct test_entry *entry; I wonder if we want to add a bit of paranoia with int count = 0; > + oidmap_iter_init(map, &iter); > + while ((entry = oidmap_iter_next(&iter))) { > + int ret; > + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > + switch (ret) { > + case -1: > + break; /* error message handled by get_oid_arbitrary_hex() */ > + case 1: > + 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)); > + break; > + case 2: > + test_msg("duplicate entry detected\n" > + " name: %s\n oid: %s\n", > + entry->name, oid_to_hex(&entry->entry.oid)); > + break; > + default: > + test_msg("BUG: invalid return value (%d) from key_val_contains()", > + ret); > + break; > + } > + } } else { count++; } > + } check_int(count, ARRAY_SIZE(key_val)); to check that we iterate over all the entries as well as checking the size of the hashmap here. > + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); Best Wishes 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(); > +}
Phillip Wood <phillip.wood123@gmail.com> wrote: > Hi Ghanshyam > > On 28/06/2024 13:20, 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. > > > > 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. > > > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > > --- > > This version addresses Phillip's review about detecting duplicates in > > oidmap when iterating over it and removing put_and_check_null() to move > > the relevant code to setup() instead. And contains some grammer fixes > > in the comment. > > This version with Junio's fixup addresses my previous comments. One more > thing occurred to me as I was reading it again > > > +static void t_iterate(struct oidmap *map) > > +{ > > + struct oidmap_iter iter; > > + struct test_entry *entry; > > I wonder if we want to add a bit of paranoia with > > int count = 0; > > > + oidmap_iter_init(map, &iter); > > + while ((entry = oidmap_iter_next(&iter))) { > > + int ret; > > + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { > > + switch (ret) { > > + case -1: > > + break; /* error message handled by get_oid_arbitrary_hex() */ > > + case 1: > > + 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)); > > + break; > > + case 2: > > + test_msg("duplicate entry detected\n" > > + " name: %s\n oid: %s\n", > > + entry->name, oid_to_hex(&entry->entry.oid)); > > + break; > > + default: > > + test_msg("BUG: invalid return value (%d) from key_val_contains()", > > + ret); > > + break; > > + } > > + } > } else { > count++; > } > > + } > check_int(count, ARRAY_SIZE(key_val)); > > to check that we iterate over all the entries as well as checking the > size of the hashmap here. > > > + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val)); Yeah, good idea. I'll include it in v3. Thanks.
Phillip Wood <phillip.wood123@gmail.com> writes: >> + } > check_int(count, ARRAY_SIZE(key_val)); > > to check that we iterate over all the entries as well as checking the > size of the hashmap here. I think check_int() macro wants the comparison operator in the middle, but other than that small typo, the suggestion sounds quite sensible. If the iterator does not yield anything, the current test would still pass. Thanks.
diff --git a/Makefile b/Makefile index 3eab701b10..2a5c70d218 100644 --- a/Makefile +++ b/Makefile @@ -809,7 +809,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 @@ -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-oidmap UNIT_TEST_PROGRAMS += t-oidtree UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-reftable-basics 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 93436a82ae..da3e69128a 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -44,7 +44,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 d9033d14e1..642a34578c 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -37,7 +37,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..13532aa98b --- /dev/null +++ b/t/unit-tests/t-oidmap.c @@ -0,0 +1,176 @@ +#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 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++){ + struct test_entry *entry; + + FLEX_ALLOC_STR(entry, name, key_val[i][1]); + if ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) { + free(entry); + break; + } + entry = oidmap_put(&map, entry); + if (!check(entry == NULL)) + free(entry); + } + + 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) +{ + for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) { + struct object_id oid; + + if (get_oid_arbitrary_hex(key_val[i][0], &oid)) + return -1; + + if (oideq(&entry->entry.oid, &oid)) { + if (!strcmp(key_val[i][1], "USED")) + return 2; + key_val[i][1] = "USED"; + return 0; + } + } + return 1; +} + +static void t_iterate(struct oidmap *map) +{ + struct oidmap_iter iter; + struct test_entry *entry; + + oidmap_iter_init(map, &iter); + while ((entry = oidmap_iter_next(&iter))) { + int ret; + if (!check_int((ret = key_val_contains(entry)), ==, 0)) { + switch (ret) { + case -1: + break; /* error message handled by get_oid_arbitrary_hex() */ + case 1: + 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)); + break; + case 2: + test_msg("duplicate entry detected\n" + " name: %s\n oid: %s\n", + entry->name, oid_to_hex(&entry->entry.oid)); + break; + default: + test_msg("BUG: invalid return value (%d) from key_val_contains()", + ret); + break; + } + } + } + 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(); +}