[libvirt] [PATCH 3/4] tests: Introduce negative versions of DO_TEST_CAPS_LATEST

[Erik Skultety]

On Tue, Dec 11, 2018 at 06:51:56PM -0500, John Ferlan wrote:
>
>
> On 12/7/18 9:47 AM, Erik Skultety wrote:
> > As commit d8266ebe161 demonstrated, it's so easy to forget to add a
> > single capability which in turn can easily fool the test suite so that
> > tests expecting a failure can fail with a different error than we
> > expected, but still making those pass. Unfortunately for commit
> > d8266ebe161, it allowed a domain with multiple OpenGL-enabled graphics
> > devices to start successfully. As a precaution measure, introduce
>
> precautionary
>
> > negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate
> > this vector from now on.
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  tests/qemuxml2argvtest.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
>
>
> Pulling the needle out of the haystack of d8266ebe1 which of the 3 new
> tests in qemuxml2argvtest was bad?  None used the TEST_CAPS_LATEST
> nomenclature and you didn't change the test in this patch, so I'm left
> wondering.  Of course, patch4 has the answer.
>
> Nothing major here, but understand coming in cold on this and reading
> the commit message is, well, confusing.
>
> I think it's simple enough to indicate you are introducing the two new
> macros to allow for test and parse failure checking. The "details" about
> the previous commit could be moved into patch 4 I suppose to show what
> you're fixing since you're not changing a test here.

Noted, will do.

>
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index e17709e7e1..528139654c 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -806,13 +806,22 @@ mymain(void)
> >  # define DO_TEST_CAPS_VER(name, ver) \
> >      DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
> >
> > -# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
> > -    DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
> > +# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \
> > +    DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags, arch, \
> >                            virHashLookup(capslatest, arch), true)
> >
> > +# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
> > +    DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) \
>
> There should not be a \ at the end of the previous line. Perhaps a
> holdover from the copy-pasta with the 'virHashLookup...' line.

Yep, nice catch :).

>
> > +
> >  # define DO_TEST_CAPS_LATEST(name) \
> >      DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
> >
> > +# define DO_TEST_FAILURE_CAPS_LATEST(name) \
>
> To follow DO_TEST_CAPS_LATEST how about
>
> DO_TEST_CAPS_LATEST_FAILURE

Better.

>
> > +    DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0)
>
> But this one never gets used - so do we really need it?  IDC, but it
> could be removed...

FAILURE was the first one I created (only then I figured the error I'm getting
is coming from the parser :/) so I thought why not add both since I'm already
creating new macros, so I'd like to keep it in regardless, might get handy
soon.

>
> > +
> > +# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
>
> and likewise
>
> DO_TEST_CAPS_LATEST_PARSE_FAILURE
>
> with the changes,

Thanks,
Erik