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

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.