Phillip Wood <phillip.wood123@xxxxxxxxx> 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@xxxxxxxxx>
> > Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx>
> > ---
> > 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.
