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

[Cole Robinson]

On 4/11/19 8:15 AM, Ján Tomko wrote:
> 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).
> 

Sure I'll add example usage for this in the next version

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

Agreed that this should go away. I think the whole WHEN_X concept should
go away: always test active and inactive paths, but the only DO_TEST
distinction is active vs inactive output is expected to be the same, vs
expected to be different. The latter case should mandate that -active
and -inactive filesnames exist, the former case doesn't look for any
state suffix. I think only the few WEHN_INACTIVE cases will need some
extra output but otherwise it's pretty compatible with the current state
of things.

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

Hopefully one day we get to a point that DO_TEST always uses latest
caps, and we have a DO_TEST_CAPS_LIST for the explicit caps list...

Thanks,
Cole