Allow testing of XML->argv conversion with using a real capability map
as used in the qemucapabilitiestest. This allows specifying the required
qemu version with the test rather than having to enumerate all the
required capabilities.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d79913dd0a..c540ce2f50 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -654,6 +654,37 @@ mymain(void)
     if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
         return EXIT_FAILURE;

+
+# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \
+                           parseFlags, gic, arch, ver) \
+    do { \
+        static struct testInfo info = { \
+            name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \
+            false, NULL \
+        }; \
+        info.skipLegacyCPUs = skipLegacyCPUs; \
+        if (testInitQEMUCaps(&info, gic) < 0) \
+            return EXIT_FAILURE; \
+        if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \
+                                          abs_srcdir "/qemucapabilitiesdata/caps_" \
+                                          ver "." arch ".xml"))) \
+            return EXIT_FAILURE; \
+        if (virTestRun("QEMU XML-2-ARGV " name, \
+                       testCompareXMLToArgv, &info) < 0) \
+            ret = -1; \
+        if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \
+                                  testCompareXMLToStartupXML, &info) < 0) \
+            ret = -1; \
+        virObjectUnref(info.qemuCaps); \
+        virObjectUnref(info.vm); \
+    } while (0)
+
+# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \
+    DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
+
+# define DO_TEST_CAPS(name, ver) \
+    DO_TEST_CAPS_FULL(name, 0, 0, ver)
+
 # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \
                       parseFlags, gic, ...) \
     do { \
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/04/2018 04:13 AM, Peter Krempa wrote:
> Allow testing of XML->argv conversion with using a real capability map
> as used in the qemucapabilitiestest. This allows specifying the required
> qemu version with the test rather than having to enumerate all the
> required capabilities.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d79913dd0a..c540ce2f50 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -654,6 +654,37 @@ mymain(void)
>      if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
>          return EXIT_FAILURE;
> 
> +
> +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \
> +                           parseFlags, gic, arch, ver) \
> +    do { \
> +        static struct testInfo info = { \
> +            name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \
> +            false, NULL \
> +        }; \
> +        info.skipLegacyCPUs = skipLegacyCPUs; \
> +        if (testInitQEMUCaps(&info, gic) < 0) \
> +            return EXIT_FAILURE; \
> +        if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \
> +                                          abs_srcdir "/qemucapabilitiesdata/caps_" \
> +                                          ver "." arch ".xml"))) \
> +            return EXIT_FAILURE; \
> +        if (virTestRun("QEMU XML-2-ARGV " name, \
> +                       testCompareXMLToArgv, &info) < 0) \
> +            ret = -1; \
> +        if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \
> +                                  testCompareXMLToStartupXML, &info) < 0) \
> +            ret = -1; \
> +        virObjectUnref(info.qemuCaps); \
> +        virObjectUnref(info.vm); \
> +    } while (0)
> +
> +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \
> +    DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)

Assumes x64_64...

> +
> +# define DO_TEST_CAPS(name, ver) \
> +    DO_TEST_CAPS_FULL(name, 0, 0, ver)
> +
>  # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \
>                        parseFlags, gic, ...) \
>      do { \
> 

Do you expect to see wide spread and future use of this model as opposed
to the existing DO_TEST model? Shouldn't at least some of the existing
tests be converted, then?

Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS
instead since DO_TEST runs the test with specific capabilities and this
new model runs the test with a specific version for a specific platform.

How do or should we enforce that when adding a new test from this point
forward that the DO_TEST_CAPS model is chosen over DO_TEST?  I suppose
there is still value in DO_TEST since one can pick and choose which CAP
is needed - although I'll freely admit that's usually a cut-n-paste from
some other test and change the name type activity.

I think I have too many questions for an R-b at this point.


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Apr 11, 2018 at 11:16:27AM -0400, John Ferlan wrote:
>
>
>On 04/04/2018 04:13 AM, Peter Krempa wrote:
>> Allow testing of XML->argv conversion with using a real capability map
>> as used in the qemucapabilitiestest. This allows specifying the required
>> qemu version with the test rather than having to enumerate all the
>> required capabilities.
>>
>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> ---
>>  tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index d79913dd0a..c540ce2f50 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -654,6 +654,37 @@ mymain(void)
>>      if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
>>          return EXIT_FAILURE;
>>
>> +
>> +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \
>> +                           parseFlags, gic, arch, ver) \
>> +    do { \
>> +        static struct testInfo info = { \
>> +            name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \
>> +            false, NULL \
>> +        }; \
>> +        info.skipLegacyCPUs = skipLegacyCPUs; \
>> +        if (testInitQEMUCaps(&info, gic) < 0) \
>> +            return EXIT_FAILURE; \
>> +        if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \
>> +                                          abs_srcdir "/qemucapabilitiesdata/caps_" \
>> +                                          ver "." arch ".xml"))) \
>> +            return EXIT_FAILURE; \
>> +        if (virTestRun("QEMU XML-2-ARGV " name, \
>> +                       testCompareXMLToArgv, &info) < 0) \
>> +            ret = -1; \
>> +        if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \
>> +                                  testCompareXMLToStartupXML, &info) < 0) \
>> +            ret = -1; \
>> +        virObjectUnref(info.qemuCaps); \
>> +        virObjectUnref(info.vm); \
>> +    } while (0)
>> +
>> +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \
>> +    DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
>
>Assumes x64_64...
>
>> +
>> +# define DO_TEST_CAPS(name, ver) \
>> +    DO_TEST_CAPS_FULL(name, 0, 0, ver)
>> +
>>  # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \
>>                        parseFlags, gic, ...) \
>>      do { \
>>
>
>Do you expect to see wide spread and future use of this model as opposed
>to the existing DO_TEST model? Shouldn't at least some of the existing
>tests be converted, then?

Yes to both.

>
>Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS
>instead since DO_TEST runs the test with specific capabilities and this
>new model runs the test with a specific version for a specific platform.
>
>How do or should we enforce that when adding a new test from this point
>forward that the DO_TEST_CAPS model is chosen over DO_TEST?  I suppose
>there is still value in DO_TEST since one can pick and choose which CAP
>is needed - although I'll freely admit that's usually a cut-n-paste from
>some other test and change the name type activity.

The problem with that approach is that we're testing against
capability combinations that might never occur in the wild.

I actually remember fixing some bug that was supposed to be covered by
a unit test, but it was missing the important capability.

Enforcement by review ought to be enough for everybody.

Though there still might be features that you can compile out even from
QEMU versions that otherwise have the capability, but we don't add
negative tests for that often.

Jano

>
>I think I have too many questions for an R-b at this point.
>
>
>John
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Sun, Apr 15, 2018 at 15:29:23 +0200, Ján Tomko wrote:
> On Wed, Apr 11, 2018 at 11:16:27AM -0400, John Ferlan wrote:
> > 
> > 
> > On 04/04/2018 04:13 AM, Peter Krempa wrote:
> > > Allow testing of XML->argv conversion with using a real capability map
> > > as used in the qemucapabilitiestest. This allows specifying the required
> > > qemu version with the test rather than having to enumerate all the
> > > required capabilities.
> > > 
> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > ---
> > >  tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)



> > Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS
> > instead since DO_TEST runs the test with specific capabilities and this
> > new model runs the test with a specific version for a specific platform.
> > 
> > How do or should we enforce that when adding a new test from this point
> > forward that the DO_TEST_CAPS model is chosen over DO_TEST?  I suppose
> > there is still value in DO_TEST since one can pick and choose which CAP
> > is needed - although I'll freely admit that's usually a cut-n-paste from
> > some other test and change the name type activity.
> 
> The problem with that approach is that we're testing against
> capability combinations that might never occur in the wild.
> 
> I actually remember fixing some bug that was supposed to be covered by
> a unit test, but it was missing the important capability.

Actually a very good example is the recent series of adding the
'discard-data' attribute to memory devices. Since the new feature is
decided with capability bits, all existing tests were still passing, but
the attribute would be added to NVDIMM memory devices which would delete
the contents.

I think we also need to add a way to take the latest caps present in the
repo to be used with the bulk of tests rather than binding it to a
specific version.

That way we can see whether the command line is stable when we add a
capability based test.

If the command line will change, the test case then can be split for the
specific version of qemu capabilities, so that the contemporary test
case still captures the new case, but the old behaviour is kept too.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Apr 11, 2018 at 11:16:27 -0400, John Ferlan wrote:
> 
> 
> On 04/04/2018 04:13 AM, Peter Krempa wrote:
> > Allow testing of XML->argv conversion with using a real capability map
> > as used in the qemucapabilitiestest. This allows specifying the required
> > qemu version with the test rather than having to enumerate all the
> > required capabilities.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index d79913dd0a..c540ce2f50 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -654,6 +654,37 @@ mymain(void)
> >      if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
> >          return EXIT_FAILURE;
> > 
> > +
> > +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \
> > +                           parseFlags, gic, arch, ver) \
> > +    do { \
> > +        static struct testInfo info = { \
> > +            name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \
> > +            false, NULL \
> > +        }; \
> > +        info.skipLegacyCPUs = skipLegacyCPUs; \
> > +        if (testInitQEMUCaps(&info, gic) < 0) \
> > +            return EXIT_FAILURE; \
> > +        if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \
> > +                                          abs_srcdir "/qemucapabilitiesdata/caps_" \
> > +                                          ver "." arch ".xml"))) \
> > +            return EXIT_FAILURE; \
> > +        if (virTestRun("QEMU XML-2-ARGV " name, \
> > +                       testCompareXMLToArgv, &info) < 0) \
> > +            ret = -1; \
> > +        if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \
> > +                                  testCompareXMLToStartupXML, &info) < 0) \
> > +            ret = -1; \
> > +        virObjectUnref(info.qemuCaps); \
> > +        virObjectUnref(info.vm); \
> > +    } while (0)
> > +
> > +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \
> > +    DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
> 
> Assumes x64_64...

Yes. Intentionally. Hipster-arches can introduce their own helpers.

> 
> > +
> > +# define DO_TEST_CAPS(name, ver) \
> > +    DO_TEST_CAPS_FULL(name, 0, 0, ver)
> > +
> >  # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \
> >                        parseFlags, gic, ...) \
> >      do { \
> > 
> 
> Do you expect to see wide spread and future use of this model as opposed
> to the existing DO_TEST model? Shouldn't at least some of the existing
> tests be converted, then?

I hope so, actually for both cases.

> Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS
> instead since DO_TEST runs the test with specific capabilities and this
> new model runs the test with a specific version for a specific platform.

Hmm, yes. I can rename it to DO_TEST_VERSION. The default will be for
x86. As said, other arches can add their own macros.

> How do or should we enforce that when adding a new test from this point
> forward that the DO_TEST_CAPS model is chosen over DO_TEST?  I suppose

I think reviewers should ask for using the new test approach for any
positive cases, so that we don't test something that is not possible in
the wild.

In cases when it's required to do so (negative tests, or weird config
tests) use of DO_TEST is justified.

> there is still value in DO_TEST since one can pick and choose which CAP
> is needed - although I'll freely admit that's usually a cut-n-paste from
> some other test and change the name type activity.
> 
> I think I have too many questions for an R-b at this point.
> 
> 
> John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

s/xm/xml/ in the summary

On Wed, Apr 04, 2018 at 10:13:55AM +0200, Peter Krempa wrote:
>Allow testing of XML->argv conversion with using a real capability map
>as used in the qemucapabilitiestest. This allows specifying the required
>qemu version with the test rather than having to enumerate all the
>required capabilities.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>

ACK

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list