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>
---
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(a)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