[libvirt] [PATCH 3/4] tests: Introduce negative versions of DO_TEST_CAPS_LATEST
eskultet at redhat.com
Wed Dec 12 08:37:14 UTC 2018
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
> > 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_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
> > +
> > +# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
> and likewise
> with the changes,
More information about the libvir-list