Re: [Qemu-devel] [PATCH] tests.acceptance: adds multi vm capability for acceptance tests

[Cleber Rosa]

On 1/14/19 10:55 AM, Caio Carrara wrote:
> This change adds the possibility to write acceptance tests with multi
> virtual machine support. It's done keeping the virtual machines objects
> stored in a test attribute (dictionary). This dictionary shouldn't be
> accessed directly but through the new method added `get_vm`. This new
> method accept a list of args (that will be added as virtual machine
> arguments) and an optional name argument. The name is the key that
> identify a single virtual machine along the test machines available. If
> a name without a machine is informed a new machine will be instantiated.
> 
> The current usage of vm in tests will not be broken by this change since
> it keeps a property called vm in the base test class. This property only
> calls the new method `get_vm` with default parameters (no args and
> 'default' as machine name).
> 
> Signed-off-by: Caio Carrara <address@hidden>
> ---
>  docs/devel/testing.rst                    | 40 ++++++++++++++++++++++-
>  tests/acceptance/avocado_qemu/__init__.py | 25 +++++++++++---
>  2 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 18e2c0868a..b97c0368bc 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -634,7 +634,45 @@ instance, available at ``self.vm``.  Because many tests 
> will tweak the
>  QEMU command line, launching the QEMUMachine (by using ``self.vm.launch()``)
>  is left to the test writer.
>  
> -At test "tear down", ``avocado_qemu.Test`` handles the QEMUMachine
> +The base test class has also support for tests with more than one
> +QEMUMachine. The way to get machines is through the ``self.get_vm()``
> +method which will return a QEMUMachine instance. The ``self.get_vm()``
> +method also accepts an optional `name` attribute so you can identify a
> +specific machine and get it more than once through the tests methods. A
> +simple and hypothetical example follows:
> +
> +.. code::
> +
> +  from avocado_qemu import Test
> +
> +
> +  class MultipleMachines(Test):
> +      """
> +      :avocado: enable
> +      """
> +      def test_multiple_machines(self):
> +          first_machine = self.get_vm()
> +          second_machine = self.get_vm()
> +          self.get_vm(name='third_machine').launch()
> +
> +          first_machine.launch()
> +          second_machine.launch()
> +
> +          first_res = first_machine.command(
> +              'human-monitor-command',
> +              command_line='info version')
> +
> +          second_res = second_machine.command(
> +              'human-monitor-command',
> +              command_line='info version')
> +
> +          third_res = self.get_vm(name='third_machine').command(
> +              'human-monitor-command',
> +              command_line='info version')
> +
> +          self.assertEquals(first_res, second_res, third_res)
> +
> +At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
>  shutdown.
>  
>  QEMUMachine
> diff --git a/tests/acceptance/avocado_qemu/__init__.py 
> b/tests/acceptance/avocado_qemu/__init__.py
> index 1e54fd5932..4c9e27feda 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -10,6 +10,7 @@
>  
>  import os
>  import sys
> +import uuid
>  
>  import avocado
>  
> @@ -42,13 +43,29 @@ def pick_default_qemu_bin():
>  
>  class Test(avocado.Test):
>      def setUp(self):
> -        self.vm = None
> +        self._vms = {}
>          self.qemu_bin = self.params.get('qemu_bin',
>                                          default=pick_default_qemu_bin())
>          if self.qemu_bin is None:
>              self.cancel("No QEMU binary defined or found in the source tree")
> -        self.vm = QEMUMachine(self.qemu_bin)
> +
> +    def _new_vm(self, *args):
> +        vm = QEMUMachine(self.qemu_bin)
> +        if args:
> +            vm.add_args(*args)
> +        return vm
> +
> +    @property
> +    def vm(self):
> +        return self.get_vm(name='default')
> +
> +    def get_vm(self, *args, name=None):
> +        if not name:
> +            name = str(uuid.uuid4())
> +        if self._vms.get(name) is None:
> +            self._vms[name] = self._new_vm(*args)
> +        return self._vms[name]
>  
>      def tearDown(self):
> -        if self.vm is not None:
> -            self.vm.shutdown()
> +        for vm in self._vms.values():
> +            vm.shutdown()
> 

Hi Caio,

I approached the newly added API purely from a user perspective, and I'm
quite happy with the results.  I've written a simple migration test
based on the API you're proposing:

https://github.com/clebergnu/qemu/commit/311ad8c8e06382616aa12403da02d001014712af

While I don't see any problems with this patch, I'd recommend you to
send another version with some concrete tests (feel free to pick that
commit above and include in your series).

Ideally, I'd also like to see some coverage for the use case of "getting
vms by name", that is:

   self.get_vm("name").make_sure_its_new()
   self.get_vm("name").do_stuff()
   other_code()
   self.get_vm("name").make_sure_its_the_same()

But that would be better done in tests for the test infrastructure code,
instead of a QEMU test.  Hopefully when get to the point of having a
formal Python module, we'll have the right place to add those.

Regards,
- Cleber.