Re: [libvirt] [PATCH 12/12] tests: qemuxml2xml: Add DO_TEST_CAPS*

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 behavior More 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",

> abs_srcdir, name, > - when == WHEN_ACTIVE ? "active" : "inactive") < 0) > + when == WHEN_ACTIVE ? "active" : "inactive", > + suffix) < 0) > goto error; > > if (!virFileExists(info->outfile)) {

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...

[...] > +# 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list