[1/1] Acceptance tests: add test for set-numa-node error handler fix

Commit Message

Cleber Rosa Nov. 21, 2018, 1:41 p.m. UTC

Commit a22528b918 fixed an issue that is exposed by means of the
"set-numa-node" QMP command (introduced in f3be67812).  This adds a
test that pretty much maps the steps documented on the fix.

Additionally, given that 'set-numa-node' is only allowed in
'preconfig' state, a specific check for that was added a separate
test.

Tests: a22528b918c7d29795129b5a64c4cb44bb57a44d
Reference: f3be67812c226162f86ce92634bd913714445420
CC: Igor Mammedov <[e-mail]>
CC: Markus Armbruster <[e-mail]>
Signed-off-by: Cleber Rosa <[e-mail]>
---
 tests/acceptance/set_numa_node.py | 41 +++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 tests/acceptance/set_numa_node.py

Comments

Markus Armbruster Nov. 21, 2018, 3:50 p.m. UTC | #1

Cleber Rosa <[e-mail]> writes:

> Commit a22528b918 fixed an issue that is exposed by means of the
> "set-numa-node" QMP command (introduced in f3be67812).  This adds a
> test that pretty much maps the steps documented on the fix.
>
> Additionally, given that 'set-numa-node' is only allowed in
> 'preconfig' state, a specific check for that was added a separate
> test.
>
> Tests: a22528b918c7d29795129b5a64c4cb44bb57a44d
> Reference: f3be67812c226162f86ce92634bd913714445420
> CC: Igor Mammedov <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> Signed-off-by: Cleber Rosa <[e-mail]>
> ---
>  tests/acceptance/set_numa_node.py | 41 +++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 tests/acceptance/set_numa_node.py
>
> diff --git a/tests/acceptance/set_numa_node.py b/tests/acceptance/set_numa_node.py
> new file mode 100644
> index 0000000000..0c55315231
> --- /dev/null
> +++ b/tests/acceptance/set_numa_node.py
> @@ -0,0 +1,41 @@
> +# Tests for QMP set-numa-node related behavior and regressions
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa <[e-mail]>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import Test
> +
> +
> +class SetNumaNode(Test):
> +    """
> +    :avocado: enable
> +    :avocado: tags=quick,numa
> +    """
> +    def test_numa_not_supported(self):
> +        self.vm.add_args('-nodefaults', '-S', '-preconfig')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +        res = self.vm.qmp('set-numa-node', type='node')
> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
> +        self.assertEqual(res['error']['class'], 'GenericError')
> +        self.assertEqual(res['error']['desc'],
> +                         'NUMA is not supported by this machine-type')

Checking the QMP command fails a certain way takes you four lines[*].
Such checks are pretty common in tests using QMP.  Consider creating a
convenience method.

> +        self.assertTrue(self.vm.is_running())
> +        self.vm.qmp('x-exit-preconfig')
> +        self.vm.shutdown()
> +        self.assertEqual(self.vm.exitcode(), 0)
> +
> +    def test_no_preconfig(self):
> +        self.vm.add_args('-nodefaults', '-S')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +        res = self.vm.qmp('set-numa-node', type='node')
> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
> +        self.assertEqual(res['error']['class'], 'GenericError')
> +        self.assertEqual(res['error']['desc'],
> +                         "The command is permitted only in 'preconfig' state")

The test looks good to me.

It could also be done as a qtest in C.  Do we have guidance on when to
use C / qtest and when to use Python / Avocado?

One possible argument for using Python more could be "tests are cheaper
to create and easier to debug that way".  Do we have evidence for that,
or at least gut feelings?

A possible argument against using Python could be much slower "make
check".  I have no idea whether that's actually the case.

Non-argument: requiring Avocado as a build dependency for testing.  I
think that's totally fine as long as it's readily available on all our
supported host platforms.  Same as for any other build dependency,
really.


[*] Would be more if you additionally checked res['error'] exists and is
a dictionary.  I'm not saying you should, just that checking @res isn't
None feels odd to me unless you do.

Cleber Rosa Nov. 21, 2018, 4:17 p.m. UTC | #2

On 11/21/18 10:50 AM, Markus Armbruster wrote:
> Cleber Rosa <[e-mail]> writes:
> 
>> Commit a22528b918 fixed an issue that is exposed by means of the
>> "set-numa-node" QMP command (introduced in f3be67812).  This adds a
>> test that pretty much maps the steps documented on the fix.
>>
>> Additionally, given that 'set-numa-node' is only allowed in
>> 'preconfig' state, a specific check for that was added a separate
>> test.
>>
>> Tests: a22528b918c7d29795129b5a64c4cb44bb57a44d
>> Reference: f3be67812c226162f86ce92634bd913714445420
>> CC: Igor Mammedov <[e-mail]>
>> CC: Markus Armbruster <[e-mail]>
>> Signed-off-by: Cleber Rosa <[e-mail]>
>> ---
>>  tests/acceptance/set_numa_node.py | 41 +++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 tests/acceptance/set_numa_node.py
>>
>> diff --git a/tests/acceptance/set_numa_node.py b/tests/acceptance/set_numa_node.py
>> new file mode 100644
>> index 0000000000..0c55315231
>> --- /dev/null
>> +++ b/tests/acceptance/set_numa_node.py
>> @@ -0,0 +1,41 @@
>> +# Tests for QMP set-numa-node related behavior and regressions
>> +#
>> +# Copyright (c) 2018 Red Hat, Inc.
>> +#
>> +# Author:
>> +#  Cleber Rosa <[e-mail]>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +from avocado_qemu import Test
>> +
>> +
>> +class SetNumaNode(Test):
>> +    """
>> +    :avocado: enable
>> +    :avocado: tags=quick,numa
>> +    """
>> +    def test_numa_not_supported(self):
>> +        self.vm.add_args('-nodefaults', '-S', '-preconfig')
>> +        self.vm.set_machine('none')
>> +        self.vm.launch()
>> +        res = self.vm.qmp('set-numa-node', type='node')
>> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
>> +        self.assertEqual(res['error']['class'], 'GenericError')
>> +        self.assertEqual(res['error']['desc'],
>> +                         'NUMA is not supported by this machine-type')
> 
> Checking the QMP command fails a certain way takes you four lines[*].
> Such checks are pretty common in tests using QMP.  Consider creating a
> convenience method.
> 

Sure, that discussion is going on.  Part of it can be seen here:

https://trello.com/c/a4wBNsGX/63-better-test-failure-output#comment-5bf448b695549b5cfa92b638

In the near future, we'd like to have a number of convenience methods,
that would optimize both test writing time, and test debugging time,
with as relevant as possible failure messages.  Something like:

   self.assertQMP(command, 'error/desc', 'NUMA is not...')

That checks for errors general QMP error conditions is something we're
planning.

>> +        self.assertTrue(self.vm.is_running())
>> +        self.vm.qmp('x-exit-preconfig')
>> +        self.vm.shutdown()
>> +        self.assertEqual(self.vm.exitcode(), 0)
>> +
>> +    def test_no_preconfig(self):
>> +        self.vm.add_args('-nodefaults', '-S')
>> +        self.vm.set_machine('none')
>> +        self.vm.launch()
>> +        res = self.vm.qmp('set-numa-node', type='node')
>> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
>> +        self.assertEqual(res['error']['class'], 'GenericError')
>> +        self.assertEqual(res['error']['desc'],
>> +                         "The command is permitted only in 'preconfig' state")
> 
> The test looks good to me.
> 
> It could also be done as a qtest in C.  Do we have guidance on when to
> use C / qtest and when to use Python / Avocado?
> 

TBH, I don't have such a guideline, and I'd probably be biased if I
tried to define one.  I've heard from more than one person, though, that
the perceived learning curve and general maintenance costs of
Python/Avocado tests seem to be way lower (IMMV, though).

> One possible argument for using Python more could be "tests are cheaper
> to create and easier to debug that way".  Do we have evidence for that,
> or at least gut feelings?
> 

Again, I'd be biased trying to answer that, and IMO this feels like an
emacs .vs. vi kind of question.  I dare to say that we're past that
point, and we should, if we receive any, just embrace contributions that
fall into this type of test.

> A possible argument against using Python could be much slower "make
> check".  I have no idea whether that's actually the case.
> 

My data is scarce, but considering that these two tests take 0.01s
seconds each on a low powered environment such as the one given by
Travis-CI:

https://travis-ci.org/clebergnu/qemu/jobs/457721176#L2051

I wouldn't be too worried about that right now.  And, these are not part
of "make check" anyway.

> Non-argument: requiring Avocado as a build dependency for testing.  I
> think that's totally fine as long as it's readily available on all our
> supported host platforms.  Same as for any other build dependency,
> really.
> 
> 
> [*] Would be more if you additionally checked res['error'] exists and is
> a dictionary.  I'm not saying you should, just that checking @res isn't
> None feels odd to me unless you do.
> 

You're right, that would indeed be an improvement.  Right now, if
res['error'] does not exist, the outcome is a test ERROR, instead of a
test FAILure.  That signals that there's a problem with the test (which
technically speaking is true), and depending how you look at it, it
could be said that it casts a shadow over the FAILure.

Having said that, we're aware of many areas in which the framework (both
in core Avocado and in the QEMU supporting code) can be improved.  The
general feeling is that we shouldn't wait until we have a supposedly
perfect environment before start writing tests.  Besides the coverage
value that those tests can bring, the improvement opportunities are best
seen during that process.

Thanks for the review and the very much valid points you raised.
- Cleber.

Wainer dos Santos Moschetta Nov. 22, 2018, 12:19 p.m. UTC | #3

On 11/21/2018 11:41 AM, Cleber Rosa wrote:
> Commit a22528b918 fixed an issue that is exposed by means of the
> "set-numa-node" QMP command (introduced in f3be67812).  This adds a
> test that pretty much maps the steps documented on the fix.
>
> Additionally, given that 'set-numa-node' is only allowed in
> 'preconfig' state, a specific check for that was added a separate
> test.
>
> Tests: a22528b918c7d29795129b5a64c4cb44bb57a44d
> Reference: f3be67812c226162f86ce92634bd913714445420
> CC: Igor Mammedov <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> Signed-off-by: Cleber Rosa <[e-mail]>
> ---
>   tests/acceptance/set_numa_node.py | 41 +++++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
>   create mode 100644 tests/acceptance/set_numa_node.py
>
> diff --git a/tests/acceptance/set_numa_node.py b/tests/acceptance/set_numa_node.py
> new file mode 100644
> index 0000000000..0c55315231
> --- /dev/null
> +++ b/tests/acceptance/set_numa_node.py
> @@ -0,0 +1,41 @@
> +# Tests for QMP set-numa-node related behavior and regressions
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa <[e-mail]>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import Test
> +
> +
> +class SetNumaNode(Test):
> +    """
> +    :avocado: enable
> +    :avocado: tags=quick,numa
> +    """
> +    def test_numa_not_supported(self):
> +        self.vm.add_args('-nodefaults', '-S', '-preconfig')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +        res = self.vm.qmp('set-numa-node', type='node')
> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
> +        self.assertEqual(res['error']['class'], 'GenericError')
> +        self.assertEqual(res['error']['desc'],
> +                         'NUMA is not supported by this machine-type')
> +        self.assertTrue(self.vm.is_running())
> +        self.vm.qmp('x-exit-preconfig')
> +        self.vm.shutdown()
> +        self.assertEqual(self.vm.exitcode(), 0)
> +
> +    def test_no_preconfig(self):
> +        self.vm.add_args('-nodefaults', '-S')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +        res = self.vm.qmp('set-numa-node', type='node')
> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
> +        self.assertEqual(res['error']['class'], 'GenericError')
> +        self.assertEqual(res['error']['desc'],
> +                         "The command is permitted only in 'preconfig' state")

There is a qtest suite of tests for NUMA configuration on 
tests/numa-test.c, including one simple test for set-numa-node (pulled 
in as https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06930.html).

So did you consider to include those cases of test on tests/numa-test.c 
instead?

- Wainer

Cleber Rosa Nov. 22, 2018, 3:32 p.m. UTC | #4

On 11/22/18 7:19 AM, Wainer dos Santos Moschetta wrote:
> 
> On 11/21/2018 11:41 AM, Cleber Rosa wrote:
>> Commit a22528b918 fixed an issue that is exposed by means of the
>> "set-numa-node" QMP command (introduced in f3be67812).  This adds a
>> test that pretty much maps the steps documented on the fix.
>>
>> Additionally, given that 'set-numa-node' is only allowed in
>> 'preconfig' state, a specific check for that was added a separate
>> test.
>>
>> Tests: a22528b918c7d29795129b5a64c4cb44bb57a44d
>> Reference: f3be67812c226162f86ce92634bd913714445420
>> CC: Igor Mammedov <[e-mail]>
>> CC: Markus Armbruster <[e-mail]>
>> Signed-off-by: Cleber Rosa <[e-mail]>
>> ---
>>   tests/acceptance/set_numa_node.py | 41 +++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 tests/acceptance/set_numa_node.py
>>
>> diff --git a/tests/acceptance/set_numa_node.py
>> b/tests/acceptance/set_numa_node.py
>> new file mode 100644
>> index 0000000000..0c55315231
>> --- /dev/null
>> +++ b/tests/acceptance/set_numa_node.py
>> @@ -0,0 +1,41 @@
>> +# Tests for QMP set-numa-node related behavior and regressions
>> +#
>> +# Copyright (c) 2018 Red Hat, Inc.
>> +#
>> +# Author:
>> +#  Cleber Rosa <[e-mail]>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +from avocado_qemu import Test
>> +
>> +
>> +class SetNumaNode(Test):
>> +    """
>> +    :avocado: enable
>> +    :avocado: tags=quick,numa
>> +    """
>> +    def test_numa_not_supported(self):
>> +        self.vm.add_args('-nodefaults', '-S', '-preconfig')
>> +        self.vm.set_machine('none')
>> +        self.vm.launch()
>> +        res = self.vm.qmp('set-numa-node', type='node')
>> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to
>> "set-numa-node"')
>> +        self.assertEqual(res['error']['class'], 'GenericError')
>> +        self.assertEqual(res['error']['desc'],
>> +                         'NUMA is not supported by this machine-type')
>> +        self.assertTrue(self.vm.is_running())
>> +        self.vm.qmp('x-exit-preconfig')
>> +        self.vm.shutdown()
>> +        self.assertEqual(self.vm.exitcode(), 0)
>> +
>> +    def test_no_preconfig(self):
>> +        self.vm.add_args('-nodefaults', '-S')
>> +        self.vm.set_machine('none')
>> +        self.vm.launch()
>> +        res = self.vm.qmp('set-numa-node', type='node')
>> +        self.assertIsNotNone(res, 'Unexpected empty QMP response to
>> "set-numa-node"')
>> +        self.assertEqual(res['error']['class'], 'GenericError')
>> +        self.assertEqual(res['error']['desc'],
>> +                         "The command is permitted only in
>> 'preconfig' state")
> 
> There is a qtest suite of tests for NUMA configuration on
> tests/numa-test.c, including one simple test for set-numa-node (pulled
> in as https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06930.html).
> 
> So did you consider to include those cases of test on tests/numa-test.c
> instead?
> 
> - Wainer
> 
> 

Hi Wainer,

Thanks for the pointer.  Answering your question, I had not looked at
tests/numa-test.c while I was writing this.

Given that one of the points of this patch is to give more (real, useful
and practical) examples of how to write tests with this framework, I
think it was a good thing.

But, this type of question, the comparison of this type of test with
other types of tests in QEMU has been recurring, so I think we should
address the question with some kind of comparison/benefits/guidelines
write up in the near future.

- Cleber.

Patch

diff --git a/tests/acceptance/set_numa_node.py b/tests/acceptance/set_numa_node.py
new file mode 100644
index 0000000000..0c55315231
--- /dev/null
+++ b/tests/acceptance/set_numa_node.py
@@ -0,0 +1,41 @@ 
+# Tests for QMP set-numa-node related behavior and regressions
+#
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+#  Cleber Rosa <[e-mail]>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+from avocado_qemu import Test
+
+
+class SetNumaNode(Test):
+    """
+    :avocado: enable
+    :avocado: tags=quick,numa
+    """
+    def test_numa_not_supported(self):
+        self.vm.add_args('-nodefaults', '-S', '-preconfig')
+        self.vm.set_machine('none')
+        self.vm.launch()
+        res = self.vm.qmp('set-numa-node', type='node')
+        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
+        self.assertEqual(res['error']['class'], 'GenericError')
+        self.assertEqual(res['error']['desc'],
+                         'NUMA is not supported by this machine-type')
+        self.assertTrue(self.vm.is_running())
+        self.vm.qmp('x-exit-preconfig')
+        self.vm.shutdown()
+        self.assertEqual(self.vm.exitcode(), 0)
+
+    def test_no_preconfig(self):
+        self.vm.add_args('-nodefaults', '-S')
+        self.vm.set_machine('none')
+        self.vm.launch()
+        res = self.vm.qmp('set-numa-node', type='node')
+        self.assertIsNotNone(res, 'Unexpected empty QMP response to "set-numa-node"')
+        self.assertEqual(res['error']['class'], 'GenericError')
+        self.assertEqual(res['error']['desc'],
+                         "The command is permitted only in 'preconfig' state")