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 or allows to use the newest capabilities present.

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

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 31218652ce..75a9d0b908 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -593,6 +593,7 @@ mymain(void)
     int ret = 0;
     char *fakerootdir;
     bool skipLegacyCPUs = false;
+    char *capsnew_x86_64 = NULL;

     if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
         fprintf(stderr, "Out of memory\n");
@@ -658,6 +659,73 @@ mymain(void)
     if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
         return EXIT_FAILURE;

+    if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata",
+                                                        "x86_64", ".xml")))
+        return EXIT_FAILURE;
+
+    VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64);
+
+
+/**
+ * The following set of macros allows testing of XML -> argv conversion with a
+ * real set of capabilities gathered from a real qemu copy. It is desired to use
+ * these for positive test cases as it provides combinations of flags which
+ * can be met in real life.
+ *
+ * The capabilities are taken from the real capabilities stored in
+ * tests/qemucapabilitiesdata.
+ *
+ * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the
+ * newest capability set. In cases when the new capabilities would remove a
+ * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER
+ * with the old version. New version of the capabilities can then be changed to
+ * the new format.
+ */
+# define DO_TEST_CAPS_INTERNAL(name, suffix, migrateFrom, flags, parseFlags, \
+                               gic, arch, capsfile) \
+    do { \
+        static struct testInfo info = { \
+            name, "." suffix, 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), \
+                                                            capsfile))) \
+            return EXIT_FAILURE; \
+        if (virTestRun("QEMU XML-2-ARGV " name "." suffix, \
+                       testCompareXMLToArgv, &info) < 0) \
+            ret = -1; \
+        virObjectUnref(info.qemuCaps); \
+        virObjectUnref(info.vm); \
+    } while (0)
+
+# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_"
+
+# define DO_TEST_CAPS_ARCH_VER_FULL(name, flags, parseFlags, gic, arch, ver) \
+    DO_TEST_CAPS_INTERNAL(name, arch "-" ver, NULL, flags, parseFlags, gic, \
+                          arch, TEST_CAPS_PATH ver "." arch ".xml")
+
+# define DO_TEST_CAPS_ARCH_VER(name, gic, arch, ver) \
+    DO_TEST_CAPS_ARCH_VER_FULL(name, 0, 0, gic, arch, ver)
+
+# define DO_TEST_CAPS_VER(name, ver) \
+    DO_TEST_CAPS_ARCH_VER(name, GIC_NONE, "x86_64", ver)
+
+# define DO_TEST_CAPS_NEW(name) \
+    DO_TEST_CAPS_INTERNAL(name, "new", NULL, 0, 0, GIC_NONE, "x86_64", capsnew_x86_64)
+
+/**
+ * The following test macros should be used only in cases when the tests require
+ * testing of some non-standard combination of capability flags
+ */
+# 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 { \
@@ -2767,6 +2835,7 @@ mymain(void)

     qemuTestDriverFree(&driver);
     VIR_FREE(fakerootdir);
+    VIR_FREE(capsnew_x86_64);

     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.16.2

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

On Wed, Apr 18, 2018 at 11:38:43AM +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 or allows to use the newest capabilities present.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
>diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>index 31218652ce..75a9d0b908 100644
>--- a/tests/qemuxml2argvtest.c
>+++ b/tests/qemuxml2argvtest.c
>@@ -593,6 +593,7 @@ mymain(void)
>     int ret = 0;
>     char *fakerootdir;
>     bool skipLegacyCPUs = false;
>+    char *capsnew_x86_64 = NULL;
>
>     if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
>         fprintf(stderr, "Out of memory\n");
>@@ -658,6 +659,73 @@ mymain(void)
>     if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
>         return EXIT_FAILURE;
>
>+    if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata",
>+                                                        "x86_64", ".xml")))
>+        return EXIT_FAILURE;
>+
>+    VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64);
>+
>+
>+/**
>+ * The following set of macros allows testing of XML -> argv conversion with a
>+ * real set of capabilities gathered from a real qemu copy. It is desired to use
>+ * these for positive test cases as it provides combinations of flags which
>+ * can be met in real life.
>+ *
>+ * The capabilities are taken from the real capabilities stored in
>+ * tests/qemucapabilitiesdata.
>+ *
>+ * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the
>+ * newest capability set. In cases when the new capabilities would remove a
>+ * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER
>+ * with the old version. New version of the capabilities can then be changed to
>+ * the new format.

The main point of these unit tests so far was to check whether changes
and refactors of the libvirt code do not affect the .args output for old
QEMUs.

Even introducting new capabilities could cause libvirt to take different
code paths, therefore we should prefer DO_TEST_CAPS_VER.
I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
new capability that needs to be handled by libvirt, the code author
should introduce a new DO_TEST_CAPS_VER test for that.
Otherwise it would just be checking whether QEMU did not break something
for us. And since the soonest we update capabilities is at the time of
QEMU freeze, that might be later than needed.


With the comment fixed to encourage DO_TEST_CAPS_VER:

Reviewed-by: Ján Tomko <jtomko@redhat.com>

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

On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
> On Wed, Apr 18, 2018 at 11:38:43AM +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 or allows to use the newest capabilities present.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> > 
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 31218652ce..75a9d0b908 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -593,6 +593,7 @@ mymain(void)
> >     int ret = 0;
> >     char *fakerootdir;
> >     bool skipLegacyCPUs = false;
> > +    char *capsnew_x86_64 = NULL;
> > 
> >     if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
> >         fprintf(stderr, "Out of memory\n");
> > @@ -658,6 +659,73 @@ mymain(void)
> >     if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
> >         return EXIT_FAILURE;
> > 
> > +    if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata",
> > +                                                        "x86_64", ".xml")))
> > +        return EXIT_FAILURE;
> > +
> > +    VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64);
> > +
> > +
> > +/**
> > + * The following set of macros allows testing of XML -> argv conversion with a
> > + * real set of capabilities gathered from a real qemu copy. It is desired to use
> > + * these for positive test cases as it provides combinations of flags which
> > + * can be met in real life.
> > + *
> > + * The capabilities are taken from the real capabilities stored in
> > + * tests/qemucapabilitiesdata.
> > + *
> > + * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the
> > + * newest capability set. In cases when the new capabilities would remove a
> > + * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER
> > + * with the old version. New version of the capabilities can then be changed to
> > + * the new format.
> 
> The main point of these unit tests so far was to check whether changes
> and refactors of the libvirt code do not affect the .args output for old
> QEMUs.

Yes, but that's not the whole story.

> Even introducting new capabilities could cause libvirt to take different
> code paths, therefore we should prefer DO_TEST_CAPS_VER.

This is actually the exact reason why I think DO_TEST_CAPS_NEW is better
than DO_TEST_CAPS_VER. If we do changes to code used in multiple places
which are gated on presence of a capability, the places using
DO_TEST_CAPS_VER could change without us being able to catch that.

> I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
> new capability that needs to be handled by libvirt, the code author
> should introduce a new DO_TEST_CAPS_VER test for that.

In such reason, the author adding the code should fork the test by
adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
then add the new capability bit. With such approach it will be even
visible which options changed.

> Otherwise it would just be checking whether QEMU did not break something
> for us. And since the soonest we update capabilities is at the time of

Actually no. It will also check that something other will not break:

https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
tried to introduce a new property for the memory-backing-file object.
This object is used in multiple places. The property is gated by a
capability bit. Our tests were not able to catch, that this would add
the property to the NVDIMM device which needs to persist the data, which
would actually break it.

While I agree that DO_TEST_CAPS_VER is very useful for checking old
command line, I think that the true value is also in DO_TEST_CAPS_NEW
when used together with DO_TEST_CAPS_VER, so when a new addition would
change some tests using DO_TEST_CAPS_NEW we should fork them to cover
both recent and old versions.

> QEMU freeze, that might be later than needed.
> 
> 
> With the comment fixed to encourage DO_TEST_CAPS_VER:

I can reword it but I disagree that DO_TEST_CAPS_VER should be used
primarily.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2018-04-18 at 13:44 +0200, Peter Krempa wrote:
> On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
> > Even introducting new capabilities could cause libvirt to take different
> > code paths, therefore we should prefer DO_TEST_CAPS_VER.
> 
> This is actually the exact reason why I think DO_TEST_CAPS_NEW is better
> than DO_TEST_CAPS_VER. If we do changes to code used in multiple places
> which are gated on presence of a capability, the places using
> DO_TEST_CAPS_VER could change without us being able to catch that.

A note on naming: both the macro and the output file should use the
world "latest" rather than "new" to be more informative and avoid
any confusion.

> > I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
> > new capability that needs to be handled by libvirt, the code author
> > should introduce a new DO_TEST_CAPS_VER test for that.
> 
> In such reason, the author adding the code should fork the test by
> adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
> then add the new capability bit. With such approach it will be even
> visible which options changed.
> 
> > Otherwise it would just be checking whether QEMU did not break something
> > for us. And since the soonest we update capabilities is at the time of
> 
> Actually no. It will also check that something other will not break:
> 
> https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
> tried to introduce a new property for the memory-backing-file object.
> This object is used in multiple places. The property is gated by a
> capability bit. Our tests were not able to catch, that this would add
> the property to the NVDIMM device which needs to persist the data, which
> would actually break it.
> 
> While I agree that DO_TEST_CAPS_VER is very useful for checking old
> command line, I think that the true value is also in DO_TEST_CAPS_NEW
> when used together with DO_TEST_CAPS_VER, so when a new addition would
> change some tests using DO_TEST_CAPS_NEW we should fork them to cover
> both recent and old versions.

I think using real-life capabilities instead of the syntetic data
we use now is a step in the right direction.

I'm still a bit uneasy about the "moving target" factor, though
I've spent some time trying to come up with possible failure
scenarios without much success...

I think ideally with each new feature the author would introduce
three tests: a negative one locked to a QEMU version that doesn't
have the required bits, a positive one locked to a QEMU version
that has them, and a positive one against the latest QEMU version.

This way, we would make sure both that new QEMU versions work
correctly with the feature and that changes in libvirt don't
affect the behavior for existing QEMU versions.

Does that sound reasonable?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
> On Wed, 2018-04-18 at 13:44 +0200, Peter Krempa wrote:
> > On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
> > > Even introducting new capabilities could cause libvirt to take different
> > > code paths, therefore we should prefer DO_TEST_CAPS_VER.
> > 
> > This is actually the exact reason why I think DO_TEST_CAPS_NEW is better
> > than DO_TEST_CAPS_VER. If we do changes to code used in multiple places
> > which are gated on presence of a capability, the places using
> > DO_TEST_CAPS_VER could change without us being able to catch that.
> 
> A note on naming: both the macro and the output file should use the
> world "latest" rather than "new" to be more informative and avoid
> any confusion.

Oh right. I knew there is a better word for this but could not figure
it out ;)

> 
> > > I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
> > > new capability that needs to be handled by libvirt, the code author
> > > should introduce a new DO_TEST_CAPS_VER test for that.
> > 
> > In such reason, the author adding the code should fork the test by
> > adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
> > then add the new capability bit. With such approach it will be even
> > visible which options changed.
> > 
> > > Otherwise it would just be checking whether QEMU did not break something
> > > for us. And since the soonest we update capabilities is at the time of
> > 
> > Actually no. It will also check that something other will not break:
> > 
> > https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
> > tried to introduce a new property for the memory-backing-file object.
> > This object is used in multiple places. The property is gated by a
> > capability bit. Our tests were not able to catch, that this would add
> > the property to the NVDIMM device which needs to persist the data, which
> > would actually break it.
> > 
> > While I agree that DO_TEST_CAPS_VER is very useful for checking old
> > command line, I think that the true value is also in DO_TEST_CAPS_NEW
> > when used together with DO_TEST_CAPS_VER, so when a new addition would
> > change some tests using DO_TEST_CAPS_NEW we should fork them to cover
> > both recent and old versions.
> 
> I think using real-life capabilities instead of the syntetic data
> we use now is a step in the right direction.
> 
> I'm still a bit uneasy about the "moving target" factor, though
> I've spent some time trying to come up with possible failure
> scenarios without much success...

There are few churn-scenarios, some avoidable, some not so much:

- When adding something which is added for a lot of configurations or all
  of them (e.g. recent adding of the new seccomp code) the change will
  generate a lot of changes.

- Machine type. We can't use any of the alias machine types in the XML
  files as they will change when new capabilities with new machine types
  are added. This can easily be avoided by using an explicit machine
  type.

While the above may induce some churn in some scenarios, I think that
the benefit of at least seeing that something changed in places where
you did not expect any change may outweigh it.

> I think ideally with each new feature the author would introduce
> three tests: a negative one locked to a QEMU version that doesn't
> have the required bits, a positive one locked to a QEMU version
> that has them, and a positive one against the latest QEMU version.

I'm considering whether it's necessary to have the one locked to the
older-but-supporting version of qemu.

My idea was that if somebody were to add test which would change
anything, the test can be forked prior to that. But it seems that it may
be a better idea to lock-in the changes right away.

> This way, we would make sure both that new QEMU versions work
> correctly with the feature and that changes in libvirt don't
> affect the behavior for existing QEMU versions.
> 
> Does that sound reasonable?

Yes. I've actually modified the test code added in this series to do
this.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2018-04-18 at 17:22 +0200, Peter Krempa wrote:
> On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
> > I think using real-life capabilities instead of the syntetic data
> > we use now is a step in the right direction.
> > 
> > I'm still a bit uneasy about the "moving target" factor, though
> > I've spent some time trying to come up with possible failure
> > scenarios without much success...
> 
> There are few churn-scenarios, some avoidable, some not so much:
> 
> - When adding something which is added for a lot of configurations or all
>   of them (e.g. recent adding of the new seccomp code) the change will
>   generate a lot of changes.
> 
> - Machine type. We can't use any of the alias machine types in the XML
>   files as they will change when new capabilities with new machine types
>   are added. This can easily be avoided by using an explicit machine
>   type.
> 
> While the above may induce some churn in some scenarios, I think that
> the benefit of at least seeing that something changed in places where
> you did not expect any change may outweigh it.

I'm not really worried about churn, but about situations where
changes to libvirt would make it behave wrongly with old versions
of QEMU but correctly with newer ones, possibly with respect to
features that are not directly touched by the changes, in ways
unforeseen by the developer.

Right now that's easy to spot because everything is locked to some
well-defined set of capabilities, but if we started using moving
targets for most tests we'd decrease coverage of older QEMU in
favor of newer QEMU over time.

> > I think ideally with each new feature the author would introduce
> > three tests: a negative one locked to a QEMU version that doesn't
> > have the required bits, a positive one locked to a QEMU version
> > that has them, and a positive one against the latest QEMU version.
> 
> I'm considering whether it's necessary to have the one locked to the
> older-but-supporting version of qemu.
> 
> My idea was that if somebody were to add test which would change
> anything, the test can be forked prior to that. But it seems that it may
> be a better idea to lock-in the changes right away.

I believe so. This would also make it so our test coverage always
increases over time.


Now, a crazy idea: what if the test author would define a baseline
version QEMU where the test passes, and the test suite would
automatically test all versions between that one and the latest?

That would mean a lot of churn whenever we add new capabilities
data, but it should be feasible to detect output files that do not
change and turn them into symlinks to reduce diffs and save disk
space.

We would also need a way to run a test only in a range of versions,
for negative testing.

Too far fetched? :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Thu, Apr 19, 2018 at 09:29:55 +0200, Andrea Bolognani wrote:
> On Wed, 2018-04-18 at 17:22 +0200, Peter Krempa wrote:
> > On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
> > > I think using real-life capabilities instead of the syntetic data
> > > we use now is a step in the right direction.
> > > 
> > > I'm still a bit uneasy about the "moving target" factor, though
> > > I've spent some time trying to come up with possible failure
> > > scenarios without much success...
> > 
> > There are few churn-scenarios, some avoidable, some not so much:
> > 
> > - When adding something which is added for a lot of configurations or all
> >   of them (e.g. recent adding of the new seccomp code) the change will
> >   generate a lot of changes.
> > 
> > - Machine type. We can't use any of the alias machine types in the XML
> >   files as they will change when new capabilities with new machine types
> >   are added. This can easily be avoided by using an explicit machine
> >   type.
> > 
> > While the above may induce some churn in some scenarios, I think that
> > the benefit of at least seeing that something changed in places where
> > you did not expect any change may outweigh it.
> 
> I'm not really worried about churn, but about situations where
> changes to libvirt would make it behave wrongly with old versions
> of QEMU but correctly with newer ones, possibly with respect to
> features that are not directly touched by the changes, in ways
> unforeseen by the developer.

Well, that is actually exactly why we need to check against the latest
capability set. If we have the locked-in feature bit set for testing, it
will catch regressions which happen with that set of capabilities. Any
regressing behaviour which would happen only with a newly added
feature will not be caught, and those are the very sneaky ones.

> Right now that's easy to spot because everything is locked to some
> well-defined set of capabilities, but if we started using moving

Well, only half of the scenarios is covered, as said above.

> targets for most tests we'd decrease coverage of older QEMU in
> favor of newer QEMU over time.

I don't think this will happen if we fork the test for a certain version
in cases when the output would change. If the output will not change we
will not have to do anything. This will prevent having a lot of test
files which don't do anything.

> > > I think ideally with each new feature the author would introduce
> > > three tests: a negative one locked to a QEMU version that doesn't
> > > have the required bits, a positive one locked to a QEMU version
> > > that has them, and a positive one against the latest QEMU version.
> > 
> > I'm considering whether it's necessary to have the one locked to the
> > older-but-supporting version of qemu.
> > 
> > My idea was that if somebody were to add test which would change
> > anything, the test can be forked prior to that. But it seems that it may
> > be a better idea to lock-in the changes right away.
> 
> I believe so. This would also make it so our test coverage always
> increases over time.
>
> Now, a crazy idea: what if the test author would define a baseline
> version QEMU where the test passes, and the test suite would
> automatically test all versions between that one and the latest?

It certainly is techincally possible as we could e.g. do a range of
versions for the macro rather than single version.

I'm only worried that it's slightly too expensive. The good thing is,
that when we'll fork the tests rigorously on any change we should get
coverage for most code paths.

The unfortunate part is that the qemuxml2argvtest has now 800 XML files
for testing (some are invalid though). If we introduce cartesian testing
with all capability dumps, it will expand the test case count 12 fold
which will tend to increase over time. (Thankfully Jano deleted a lot of
old crap though)

> That would mean a lot of churn whenever we add new capabilities
> data, but it should be feasible to detect output files that do not
> change and turn them into symlinks to reduce diffs and save disk
> space.

Actually if a test is stable (which requires it to be as minimal as
possible) in a range of versions, adding new capabilities should be for
free. We can test all versions in a range against the same output file.

It should only change when adding a new feature.

This only requires forking the test file if a change modifies anything
retroactively.

> We would also need a way to run a test only in a range of versions,
> for negative testing.

Well, the same as for positive testing. If we have infrastructure to
test a version range, then this should be possible as well. I'm not sure
though whether it's worth it at all.

> Too far fetched? :)

Slightly. But certainly has some value. Unfortunately I can't put in
much more time besides this initial idea, since I'm busy with the
blockdev stuff.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Apr 18, 2018 at 01:44:23PM +0200, Peter Krempa wrote:
>On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
>> On Wed, Apr 18, 2018 at 11:38:43AM +0200, Peter Krempa wrote:

[ ... yaba daba doo ... ]

>> I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
>> new capability that needs to be handled by libvirt, the code author
>> should introduce a new DO_TEST_CAPS_VER test for that.
>
>In such reason, the author adding the code should fork the test by
>adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
>then add the new capability bit. With such approach it will be even
>visible which options changed.
>

Okay.

>> Otherwise it would just be checking whether QEMU did not break something
>> for us. And since the soonest we update capabilities is at the time of
>
>Actually no. It will also check that something other will not break:
>
>https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
>tried to introduce a new property for the memory-backing-file object.
>This object is used in multiple places. The property is gated by a
>capability bit. Our tests were not able to catch, that this would add
>the property to the NVDIMM device which needs to persist the data, which
>would actually break it.
>

Seems _NEW wins here.

>While I agree that DO_TEST_CAPS_VER is very useful for checking old
>command line, I think that the true value is also in DO_TEST_CAPS_NEW
>when used together with DO_TEST_CAPS_VER, so when a new addition would
>change some tests using DO_TEST_CAPS_NEW we should fork them to cover
>both recent and old versions.
>
>> QEMU freeze, that might be later than needed.
>>
>>
>> With the comment fixed to encourage DO_TEST_CAPS_VER:
>
>I can reword it but I disagree that DO_TEST_CAPS_VER should be used
>primarily.

ACK then, as long as you incorporate Andrea's suggestion to call it
_LATEST

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