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

Andrea Bolognani abologna at redhat.com
Thu Apr 11 10:44:16 UTC 2019


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




More information about the libvir-list mailing list