Re: [PATCH 0/7] qemu XML testing improvements, part 3 - xmlout->xmlout testing and fixes
On Tue, Mar 19, 2024 at 21:31:52 +0100, Michal Prívozník wrote:
On 1/12/24 17:05, Peter Krempa wrote:
> The main goal of part 3 is to add testing based on parsing of the
> libvirt-formatted files from tests/qemuxml2xmloutdata and formatting
> them back, and checking that they are identical.
Firstly, sorry for resurrecting an old thread.
Secondly, sorry for hijacking it. BUT.
There were patches sent to the list recently [1] and I realized, the way
we usually used to do things is disturbed. I mean, whenever new device,
device knob, ... was introduced the patch series consisted (roughly) of
the following patches:
1) config, XML parser & formatter, RNG, docs AND xml2xml test case,
2) qemu caps
3) qemu cmd line AND xml2argv test case.
Now, since we have this one huge qemuxmlconftest.c it's not as easy. If
I try to introduce a test case in 1), the test case fails as there's no
corresponding .args file.
You can add a corresponding .args file that will not yet have the
commandline bits added by the patches, as the implementation is missing.
This means that the corresponding patch adding the qemu code will need
to contain the corresponding .args change.
Fair, but also not fair - the feature is not
finished at the time of 1) so .args shouldn't even be considered.
Well, it still covers what the code does at that point, so at the very
least in theory it makes sense. In practice, it possibly looks a bit
awkward, but the tests can still be added in a separate commit after
both the XML and qemu bits are implemented.
As long as the patches test the code I don't really have a problem with
that.
But if
a test case is added at step 3) - well, then anybody backporting 1)
won't get the xml2xml test case. Pity. I mean, that's the whole point we
split the change into "frontend" and "backend", right?
This case would be a problem only if a patchset would add
implementations in two hypervisor drivers at same time and you'd decide
to backport only the non-qemu one. In such case the qemu XML coverage
would be missing.
Reallistically that won't happen, but if it did, the corresponding
patches to the other hypervisor driver should add their own tests which
you really should bacport or you can also add genericxml2xmltest cases.
In quite a lot of cases the qemuxml2xmltest case was also added in a
separate commit which is totally fine and the 'backport' case above
would not cover that one either.
Okay, you may add step 4), which introduces new qemuxmlconftest.c
test
case. But it bundles both xml2xml AND xml2argv steps rendering it yet
again unsuitable for backport.
I don't see how that would be unsuitable for backport. If you are
backporting a qemu feature, then the qemu* tests are relevant. Otherwise
you should be backporting the corresponding tests at least for that
hypervisor you are adding.
One way out might be to add new testcases to genericxml2xmltest.c,
but
somehow that feels wrong.
Why? Genericxml2xml test is there for all generic configs. It's just
that most code usually adds a qemu implementation and it's both more
convenient and sufficient to excercise all things to add just the
qemuxmlconftest case.
P.S: 37% qemuxml2argv test cases did not have a corresponding
qemuxml2xmltest case at the time when I've converted it over. Additional
few were actually testing something else due to a mismatch in
capabilities.