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

Thu Apr 11 12:15:56 UTC 2019

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

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190411/fdc6fb10/attachment-0001.sig>