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

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(a)redhat.com&gt; > --- > 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.

> 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.

> + > # 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

> + 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...

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