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

[John Ferlan]

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.

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

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> +    DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0)
> +
>  /**
>   * The following test macros should be used only in cases when the tests require
>   * testing of some non-standard combination of capability flags
>