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