[libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
qemuxml2argv already supports the ability to include test cases that are known not to make it past XML parsing, and since we want to keep qemuxml2xml in sync with it as much as possible, we need to implement this missing feature. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2xmltest.c | 104 +++++++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 37 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c74614c..66729fa 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -28,6 +28,10 @@ enum { WHEN_BOTH = 3, }; +typedef enum { + FLAG_EXPECT_PARSE_ERROR = 1 << 0, +} virQemuXML2XMLTestFlags; + struct testInfo { char *inName; char *outActiveName; @@ -36,6 +40,8 @@ struct testInfo { virBitmapPtr activeVcpus; virQEMUCapsPtr qemuCaps; + + virQemuXML2XMLTestFlags flags; }; static int @@ -55,12 +61,15 @@ static int testXML2XMLActive(const void *opaque) { const struct testInfo *info = opaque; + testCompareDomXML2XMLResult flags = TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS; + + if (info->flags & FLAG_EXPECT_PARSE_ERROR) + flags = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName, info->outActiveName, true, qemuXML2XMLActivePreFormatCallback, - opaque, 0, - TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); + opaque, 0, flags); } @@ -68,11 +77,14 @@ static int testXML2XMLInactive(const void *opaque) { const struct testInfo *info = opaque; + testCompareDomXML2XMLResult flags = TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS; + + if (info->flags & FLAG_EXPECT_PARSE_ERROR) + flags = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName, info->outInactiveName, false, - NULL, opaque, 0, - TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); + NULL, opaque, 0, flags); } @@ -195,6 +207,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES))) { + if (data->flags & FLAG_EXPECT_PARSE_ERROR) + goto ok; VIR_TEST_DEBUG("Failed to parse domain status XML:\n%s", source); goto cleanup; } @@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque) ret = 0; + ok: + if (data->flags & FLAG_EXPECT_PARSE_ERROR) { + if (ret < 0) { + VIR_TEST_DEBUG("Got expected error"); + ret = 0; + } else { + VIR_TEST_DEBUG("Error expected but there wasn't any"); + ret = -1; + } + } + cleanup: xmlKeepBlanksDefault(keepBlanksDefault); xmlFreeDoc(xml); @@ -249,7 +274,8 @@ static int testInfoSet(struct testInfo *info, const char *name, int when, - int gic) + int gic, + virQemuXML2XMLTestFlags flags) { if (!(info->qemuCaps = virQEMUCapsNew())) goto error; @@ -296,6 +322,8 @@ testInfoSet(struct testInfo *info, } } + info->flags = flags; + return 0; error: @@ -335,9 +363,9 @@ mymain(void) /* TODO: test with format probing disabled too */ driver.config->allowDiskFormatProbing = true; -# define DO_TEST_FULL(name, when, gic, ...) \ +# define DO_TEST_FULL(name, when, gic, flags, ...) \ do { \ - if (testInfoSet(&info, name, when, gic) < 0) { \ + if (testInfoSet(&info, name, when, gic, flags) < 0) { \ VIR_TEST_DEBUG("Failed to generate test data for '%s'", name); \ return -1; \ } \ @@ -364,8 +392,10 @@ mymain(void) # define NONE QEMU_CAPS_LAST # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, __VA_ARGS__) + DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, 0, __VA_ARGS__) +# define DO_TEST_PARSE_ERROR(name, ...) \ + DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, FLAG_EXPECT_PARSE_ERROR, __VA_ARGS__) /* Unset or set all envvars here that are copied in qemudBuildCommandLine @@ -493,7 +523,7 @@ mymain(void) QEMU_CAPS_SCSI_DISK_WWN); DO_TEST("disk-mirror-old", NONE); DO_TEST("disk-mirror", NONE); - DO_TEST_FULL("disk-active-commit", WHEN_ACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("disk-active-commit", WHEN_ACTIVE, GIC_NONE, 0, NONE); DO_TEST("graphics-listen-network", NONE); DO_TEST("graphics-vnc", NONE); DO_TEST("graphics-vnc-websocket", NONE); @@ -571,7 +601,7 @@ mymain(void) DO_TEST("channel-virtio", NONE); DO_TEST("channel-virtio-state", NONE); - DO_TEST_FULL("channel-unix-source-path", WHEN_INACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("channel-unix-source-path", WHEN_INACTIVE, GIC_NONE, 0, NONE); DO_TEST("hostdev-usb-address", NONE); DO_TEST("hostdev-pci-address", NONE); @@ -644,17 +674,17 @@ mymain(void) DO_TEST("blkdeviotune-max-length", NONE); DO_TEST("controller-usb-order", NONE); - DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, 0, NONE); + DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, 0, NONE); + DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, 0, NONE); + DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, GIC_NONE, 0, NONE); DO_TEST("seclabel-static", NONE); - DO_TEST_FULL("seclabel-static-labelskip", WHEN_ACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("seclabel-static-labelskip", WHEN_ACTIVE, GIC_NONE, 0, NONE); DO_TEST("seclabel-none", NONE); DO_TEST("seclabel-dac-none", NONE); DO_TEST("seclabel-dynamic-none", NONE); DO_TEST("seclabel-device-multiple", NONE); - DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, GIC_NONE, 0, NONE); DO_TEST("numad-static-vcpu-no-numatune", NONE); DO_TEST("disk-scsi-lun-passthrough-sgio", @@ -1088,27 +1118,27 @@ mymain(void) QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_BOOTINDEX); - DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_BOTH, NONE); + DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, 0, NONE); + DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, 0, NONE); + DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, GIC_V3, 0, NONE); + DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, 0, NONE); + DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, 0, NONE); + DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, 0, NONE); + DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, 0, NONE); + DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, 0, NONE); + DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, 0, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, 0, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, 0, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, 0, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_BOTH, 0, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_NONE, 0, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V2, 0, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V3, 0, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_BOTH, 0, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_NONE, 0, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V2, 0, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V3, 0, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_BOTH, 0, NONE); DO_TEST("memory-hotplug", NONE); DO_TEST("memory-hotplug-nonuma", NONE); -- 2.7.5
On Tue, Jul 04, 2017 at 15:13:51 +0200, Andrea Bolognani wrote:
qemuxml2argv already supports the ability to include test cases that are known not to make it past XML parsing, and since we want to keep qemuxml2xml in sync with it as much as possible, we need to implement this missing feature.
I must say that I don't really see the point to have them fail twice ... I'd understand if this would be added to the genericxml2xmltest (if it's not there)
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2xmltest.c | 104 +++++++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 37 deletions(-)
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c74614c..66729fa 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c
@@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
ret = 0;
+ ok:
This is not an okay name for a label.
+ if (data->flags & FLAG_EXPECT_PARSE_ERROR) { + if (ret < 0) { + VIR_TEST_DEBUG("Got expected error"); + ret = 0;
The rest looks good, but the explanation for need to do this is not that convincing, since testing the same thing twice does not really make sense. I'd make more sense if you plan to move the cases which are supposed to fail here, since this is the test dealing with XML parsing.
On Fri, 2017-07-07 at 12:48 +0200, Peter Krempa wrote:
qemuxml2argv already supports the ability to include test cases that are known not to make it past XML parsing, and since we want to keep qemuxml2xml in sync with it as much as possible, we need to implement this missing feature. I must say that I don't really see the point to have them fail twice ... I'd understand if this would be added to the genericxml2xmltest (if it's not there)
We have a lot of test cases with xml2argv coverage but no xml2xml coverage, and vice versa. The obvious solution to that is to, in due time, make it so you only need to list input file and capabilities once to have them tested both for xml2argv and xml2xml, and this is a step towards that still pretty far away goal.
@@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque) ret = 0; + ok: This is not an okay name for a label.
It's clearly an "ok" name though :P I borrowed it from testCompareXMLToArgv() in xml2argv, but I don't have a problem changing it to something you think is more suitable. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jul 07, 2017 at 13:00:10 +0200, Andrea Bolognani wrote:
On Fri, 2017-07-07 at 12:48 +0200, Peter Krempa wrote:
qemuxml2argv already supports the ability to include test cases that are known not to make it past XML parsing, and since we want to keep qemuxml2xml in sync with it as much as possible, we need to implement this missing feature. I must say that I don't really see the point to have them fail twice ... I'd understand if this would be added to the genericxml2xmltest (if it's not there)
We have a lot of test cases with xml2argv coverage but no xml2xml coverage, and vice versa. The obvious solution to
Well. If the file fails to parse, I don't see why it would make sense to make it fail to parse twice. The both tests in their success paths have value. Failing to parse the same document twice does not.
that is to, in due time, make it so you only need to list input file and capabilities once to have them tested both for xml2argv and xml2xml, and this is a step towards that still pretty far away goal.
This is certainly a good worthwhile goal. I just don't really see how splitting the testing of unparsable documents into two places or duplicating it will help your effort to simplify it. The combined test will have to be renamed anyways and the xml2argv test has the capability flags, so that seems as a better point to start the rename.
@@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque) ret = 0; + ok: This is not an okay name for a label.
It's clearly an "ok" name though :P
I borrowed it from testCompareXMLToArgv() in xml2argv, but I don't have a problem changing it to something you think is more suitable.
I'd prefer 'done'. 'ok' seems too short. Also the test still may fail at that point, so 'done' should not imply success (hopefully).
On Fri, 2017-07-07 at 13:15 +0200, Peter Krempa wrote:
that is to, in due time, make it so you only need to list input file and capabilities once to have them tested both for xml2argv and xml2xml, and this is a step towards that still pretty far away goal. This is certainly a good worthwhile goal. I just don't really see how splitting the testing of unparsable documents into two places or duplicating it will help your effort to simplify it.
For some reason I was convinced that having the test in both files would make the task of unifying the list down the line easier, which is of course complete rubbish. So forget this patch ever existed. I'll add the test case to genericxml2xmltest instead, as you suggested. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Peter Krempa