[libvirt] [PATCH v2 3/5] test: Introduce testing of virStorageUtilGlusterExtractPoolSources

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
> @@ -352,7 +354,7 @@ test_programs += nwfilterxml2firewalltest
>  endif WITH_NWFILTER
> 
>  if WITH_STORAGE
> -test_programs += storagevolxml2argvtest
> +test_programs += storagevolxml2argvtest virstorageutiltest

Since you have to touch these lines regardless, please turn
this...

[...]
> @@ -859,6 +861,13 @@ genericxml2xmltest_LDADD = $(LDADDS)
> 
> 
>  if WITH_STORAGE
> +virstorageutiltest_SOURCES = \
> +	virstorageutiltest.c testutils.c testutils.h

... and this ...

> +virstorageutiltest_LDADD = \
> +	../src/libvirt_driver_storage_impl.la \
> +	$(LDADDS) \
> +	$(NULL)
> +
>  storagevolxml2argvtest_SOURCES = \
>      storagevolxml2argvtest.c \
>      testutils.c testutils.h
> @@ -867,7 +876,7 @@ storagevolxml2argvtest_LDADD = \
>  	../src/libvirt_driver_storage_impl.la $(LDADDS)
> 
>  else ! WITH_STORAGE
> -EXTRA_DIST += storagevolxml2argvtest.c
> +EXTRA_DIST += storagevolxml2argvtest.c virstorageutiltest.c

... and finally this into proper, one-file-per-line,
$(NULL)-terminated file lists as a courtesy to the next
developer :)

You should also be able to use $(qemu_LDADDS) instead of
spelling out ../src/libvirt_driver_storage_impl.la above.

[...]
> +          <brick uuid="a6f5ddea-bc6a-44db-ae1d-5aa1db743490">virt-gluster-node1:/bricks/brick1/brick<name>virt-gluster-
node1:/bricks/brick1/brick</name><hostUuid>a6f5ddea-bc6a-44db-ae1d-5aa1db743490</hostUuid><isArbiter>0</isArbiter></brick>
> +          <brick uuid="f4ab9fb1-44ec-443b-8783-e5f70ed78da3">virt-gluster-node2:/bricks/brick1/brick<name>virt-gluster-
node2:/bricks/brick1/brick</name><hostUuid>f4ab9fb1-44ec-443b-8783-e5f70ed78da3</hostUuid><isArbiter>0</isArbiter></brick>

Does Gluster's <brick> element really contain a bare text
node followed by a bunch of other elements? Ewww.

[...]
> +#define DO_TEST_GLUSTER_LOOKUP_FULL(testname, sffx, testnetfs, pooltype)       \
> +    do {                                                                       \
> +        struct testGlusterLookupParseData data;                                \
> +        data.srcxml = abs_srcdir "/virstorageutildata/"                        \
> +                      "gluster-parse-" testname "-src.xml";                    \
> +        data.dstxml = abs_srcdir "/virstorageutildata/"                        \
> +                      "gluster-parse-" testname "-" sffx  ".xml";              \

There's a spurious space between sffx and ".xml".

> +        data.netfs = testnetfs;                                                \
> +        data.type = pooltype;                                                  \
> +        if (virTestRun("gluster lookup " sffx " " testname,                    \
> +                       testGlusterLookupParse, &data) < 0)                     \
> +            ret = -1;                                                          \
> +    } while (0)
> +
> +#define DO_TEST_GLUSTER_LOOKUP_NATIVE(testname)                                \
> +    DO_TEST_GLUSTER_LOOKUP_FULL(testname, "native", false, VIR_STORAGE_POOL_GLUSTER)
> +#define DO_TEST_GLUSTER_LOOKUP_NETFS(testname)                                 \
> +    DO_TEST_GLUSTER_LOOKUP_FULL(testname, "netfs", true, VIR_STORAGE_POOL_NETFS)
> +
> +    DO_TEST_GLUSTER_LOOKUP_NATIVE("basic");
> +    DO_TEST_GLUSTER_LOOKUP_NETFS("basic");

Between GLUSTER_LOOKUP, gluster-parse and
GlusterLookupParse the naming is a bit all over the place.

Do you think you could make it more consistent? Especially
since none of those seems to have any direct connection with
virStorageUtilGlusterExtractPoolSources(), the API being
tested.

[...]
> +#undef DO_TEST_GLUSTER_LOOKUP_NATIVE
> +#undef DO_TEST_GLUSTER_LOOKUP_NETFS
> +#undef DO_TEST_GLUSTER_LOOKUP_FULL

The #undefs are a bit unnecessary in this context, I'd leave
them out.

-- 
Andrea Bolognani / Red Hat / Virtualization