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