Re: [Qemu-devel] [PATCH 5/8] VNC Acceptance test: use UNIX domain sockets to avoid port collisions

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

On 06/07/2019 12:22 PM, Cleber Rosa wrote:
> While running in parallel, the VNC tests that use a TCP port easily
> collide.  There's a number of possibilities to reduce the probability
> of collisions, but none that completely prevents it from happening.
>
> So, to avoid those collisions, and given that the scope of the tests
> are really not related to nature of the socket type, let's switch to
> UNIX domain sockets created in temporary directories.
>
> Note: the amount of boiler plate code is far from the ideal, but it's
> related to the fact that a test "workdir"[1] attribute can not be used
> here, because of the 108 bytes limitation of the UNIX socket path (see
> ad9579aaa16). There's a fair assumption here that the temporary
> directory returned by Python's tempfile.mkdtemp() won't be anywhere
> close to 100 bytes.
>
> [1] https://avocado-framework.readthedocs.io/en/68.0/api/test/avocado.html#avocado.Test.workdir
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/vnc.py | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/vnc.py b/tests/acceptance/vnc.py
> index 064ceabcc1..675fd507ed 100644
> --- a/tests/acceptance/vnc.py
> +++ b/tests/acceptance/vnc.py
> @@ -8,6 +8,10 @@
>   # This work is licensed under the terms of the GNU GPL, version 2 or
>   # later.  See the COPYING file in the top-level directory.
>   
> +import os
> +import tempfile
> +import shutil
> +
>   from avocado_qemu import Test
>   
>   
> @@ -34,8 +38,16 @@ class Vnc(Test):
>           self.assertEqual(set_password_response['error']['desc'],
>                            'Could not set password')
>   
> +class VncUnixSocket(Test):
> +
> +    def setUp(self):
> +        super(VncUnixSocket, self).setUp()
> +        self.socket_dir = tempfile.mkdtemp()
> +        self.socket_path = os.path.join(self.socket_dir, 'vnc-socket')
> +
>       def test_vnc_change_password_requires_a_password(self):
> -        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
> +        self.vm.add_args('-nodefaults', '-S',
> +                         '-vnc', 'unix:%s' % self.socket_path)
>           self.vm.launch()
>           self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
>           set_password_response = self.vm.qmp('change',
> @@ -49,7 +61,8 @@ class Vnc(Test):
>                            'Could not set password')
>   
>       def test_vnc_change_password(self):
> -        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password')
> +        self.vm.add_args('-nodefaults', '-S',
> +                         '-vnc', 'unix:%s,password' % self.socket_path)
>           self.vm.launch()
>           self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
>           set_password_response = self.vm.qmp('change',
> @@ -57,3 +70,6 @@ class Vnc(Test):
>                                               target='password',
>                                               arg='new_password')
>           self.assertEqual(set_password_response['return'], {})
> +
> +    def tearDown(self):
> +        shutil.rmtree(self.socket_dir)

You missed to call super's tearDown in order to gently shutdown all VM 
created in by the tests. Other than that, it looks good to me.

- Wainer