[libvirt] [PATCH] tests: Unit tests for internal hash APIs

[Jiri Denemark]

On Fri, Apr 15, 2011 at 16:31:15 -0600, Eric Blake wrote:
> On 04/15/2011 02:04 PM, Jiri Denemark wrote:
> > This is a basic set of tests for testing removals of hash entries during
> > iteration.
> > ---
> > More tests for all other hash APIs will come on Monday.
> 
> Such as a test that gets 8 collisions into a single bucket to force hash
> table growth

Yes.

> , or a test of custom hasher/comparator functions?

Not yet, but I'll add it, thanks for mentioning that :-)

> > +++ b/tests/hashdata.h
> > @@ -0,0 +1,33 @@
> > +const char *uuids[] = {
> > +/* [ 46] */ "f17494ba-2353-4af0-b1ba-13680858edcc",
> > +            "64ab4e78-1b6e-4b88-b47f-2def46c79a86",
> > +            "f99b0d59-ecff-4cc6-a9d3-20159536edc8",
> > +/* [ 75] */ "e1bfa30f-bc0b-4a24-99ae-bed7f3f21a7b",
> > +            "acda5fa0-58de-4e1e-aa65-2124d1e29c54",
> > +/* [ 76] */ "5f476c28-8f26-48e0-98de-85745fe2f304",
> 
> Looks suspiciously like you used gdb to dump an existing hash structure
> on a machine with lots of VMs :)  Works for me.

Not really, I just generated a bunch of UUIDs with uuidgen, used them within
this test to create a hash, dumped that in gdb and replaced the original list
with this dump :-) The full set of UUIDs will come with the additional tests
since these two test didn't really require so many entries in a hash table.

> 
> > +static virHashTablePtr
> > +testHashInit(int size)
> > +{
> > +    virHashTablePtr hash;
> > +    int i;
> > +
> > +    if (!(hash = virHashCreate(size, NULL)))
> > +        return NULL;
> > +
> > +    /* entires are added in reverse order so that they will be linked in
> > +     * collision list in the same order as in the uuids array
> 
> We're abusing an an internal detail.  So good to have the comment - if
> the test breaks because of another rewrite, but the only breakage is due
> to a different link order in each bucket, then I'm okay modifying the
> test at that point, and meanwhile it justifies our abuse (that is, no
> change needed).

The order doesn't matter much. Actually the comment isn't even right for
current hash code. It's correct only after applying the hash rewrite. Making
sure link order matches the uuids array just helps with designing the tests.
E.g., when removing entries, one can easily say which entry is the first, last
or in the middle of the list and can select appropriate uuids to test removing
from different places in the list.

> > +#define DO_TEST_DATA(name, cmd, data)                               \
> > +    DO_TEST_FULL(name "(" #data ")",                                \
> > +                 cmd,                                               \
> > +                 testHash ## cmd ## data,                           \
> > +                 testHashCount ## cmd ## data)
> > +
> > +    DO_TEST_DATA("Remove in ForEach", RemoveForEach, Some);
> > +    DO_TEST_DATA("Remove in ForEach", RemoveForEach, All);
> > +
> > +    return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
> 
> Looks like a good test; it certainly would have caught the previous bugs
> before the most recent hash.c fixes.

Yes, of course I tested this test with older libvirt code without the hash
fixes and the test just segfaults.

> ACK.

Thanks, pushed.

Jirka