On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed.Please have each of these changes as separate preparatory commits.One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename. This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behaviorMore on this later. [...]static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix)'suffix' should come before 'when', to match the corresponding function in xml2argv. Additionally, we're passing 'name' to the function here but we're storing it inside 'info' in xml2argv - we should be doing the same here. Please make that change as a separate preparatory commit. [...]if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml",
I'd definitely put another minus between these suffixes (also, I'd like to see them in use).
abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error; if (!virFileExists(info->outfile)) {
As for this virFileExists - I really dislike it. It is on my TODO list(TM) to change this to either: A) specify exactly which output files the test needs by using the appropriate DO_TEST macro or B) add a lot of symlinks for the expected output to the output directory.
Missing from the context, but immediately after this line we have:
if (virAsprintf(&info->outfile,
"%s/qemuxml2xmloutdata/%s.xml",
abs_srcdir, name) < 0)
...
We should format 'suffix' here too.
---
Following up from the commit message: I was wondering about the way
this test works, and discussing it with Jano (CC'd since he had his
own opinion on the matter).
There are several situations we can run into with these tests:
1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and
WHEN_INACTIVE cases match;
2) same as the above, but the outputs don't match;
3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE.
Case 1) covers the vast majority of existing test cases.
As for output files, I would expect respectively
1) a single output file, with no suffix;
2) two output files with different suffixes;
3) a single output file with the corresponding -inactive or
-active suffix.
The problem with the current algorithm is that, when
VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new
test case so no output files exist yet, you'll end up with
1) the expected output, yay!;
2) failure, because both WHEN variants will write the (different)
output to the unsuffixed file and step on each other's toes
every time. This is annoying but ultimately okay, because the
developer can break the loop simply by touching the suffixed
files before running the test program;
3) the unsuffixed file being created instead of the suffixed one.
The third scenario is suboptimal but not necessarily a very big deal
either.
One way to get rid of the above would be to pass an extra flags that
controls whether falling back to the unsuffixed output files should
be considered at all: the default would be to pass it for WHEN_BOTH,
so that scenario 1) would be covered, and not to pass it for
WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few
test cases that fall into scenario 2) would have to go for a more
verbose macro and *not* pass the flag manually. I feel like that
would be acceptable given that most tests cases fall into 1) anyway.
---
None of the above is really connected to whether or not we should
use 'suffix' as I suggested earlier: we should definitely format it,
even if it causes test suite churn. Not only that: you should also
make sure...
The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests to coexist with the ones with enumerated capabilities. But it also contains the architecture, so even if -latest would be the prevailing case, I'd rather format it anyway. Jano
[...]+# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__)... you copy DO_TEST_CAPS_ARCH_VER() and DO_TEST_CAPS_VER() from xml2argv too, so that you can later introduce... [...]+ DO_TEST_CAPS_LATEST("virtio-transitional"); + DO_TEST_CAPS_LATEST("virtio-non-transitional");DO_TEST_CAPS_VER("virtio-transitional", "3.1.0"); DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); and friends like in xml2argv, too. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
