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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
