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

[Cleber Rosa <crosa@redhat.com>]

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 <imammedo@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   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 <crosa@redhat.com>
>> +#
>> +# 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.