Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests

[Cleber Rosa]

On 05/30/2018 12:29 PM, Eduardo Habkost wrote:
> On Wed, May 30, 2018 at 01:57:05PM +0100, Stefan Hajnoczi wrote:
>> On Tue, May 29, 2018 at 03:37:28PM -0400, Cleber Rosa wrote:
>>> This patch adds a few simple behavior tests for VNC.
>>>
>>> Signed-off-by: Cleber Rosa <address@hidden>
>>> ---
>>>  tests/acceptance/vnc.py | 60 +++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>  create mode 100644 tests/acceptance/vnc.py
>>>
>>> diff --git a/tests/acceptance/vnc.py b/tests/acceptance/vnc.py
>>> new file mode 100644
>>> index 0000000000..b1ef9d71b1
>>> --- /dev/null
>>> +++ b/tests/acceptance/vnc.py
>>> @@ -0,0 +1,60 @@
>>> +# Simple functional tests for VNC functionality
>>> +#
>>> +# Copyright (c) 2018 Red Hat, Inc.
>>> +#
>>> +# Author:
>>> +#  Cleber Rosa <address@hidden>
>>> +#
>>> +# 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 Vnc(Test):
>>> +    """
>>> +    :avocado: enable
>>> +    :avocado: tags=vnc,quick
>>> +    """
>>> +    def test_no_vnc(self):
>>> +        self.vm.add_args('-nodefaults', '-S')
>>> +        self.vm.launch()
>>
>> If this pattern becomes very common maybe vm.launch() should become
>> vm.launch(*extra_args) to make this a one-liner:
>>
>>   self.vm.launch('-nodefaults', '-S')
> 
> I think this was suggested in the past:
> 
>   self.vm.add_args(...).launch()
> 
> This style would be useful if we add other methods that change
> QEMU options in addition to raw add_args().  e.g.:
> 
>   self.vm.add_args(...).add_console(...).add_drive(...).launch()
> 

Yes, this is an old discussion indeed.  IMO, method chaining can look
attractive at first, but it will quickly bring a few new issues to be
dealt with.

For once, it brings the assumption that it can be done for all methods.
 Using the previous example I'd expect to be able to do:

   self.vm.add_args(...).launch(...).migrate(...).shutdown()

Which seems fine, but now the code is plagued with "return self" statements.

Then, code style and indentation questions will quickly arise.  Either this:

   self.vm.add_args('-nodefaults', '-S').add_console('isa-serial') \
          .add_drive(file='/path/to/file, format='raw').launch() \
          .shutdown()

Or this:

   (self.vm.add_args('-nodefaults', '-S').add_console('isa-serial')
           .add_drive(file='/path/to/file, format='raw').launch()
           .shutdown())

Looks just as bad to me.  The non-method chaining syntax would look like:

   self.vm.add_args('-nodefaults', '-S')
   self.vm.add_console('isa-serial')
   self.vm.add_drive(file='/path/to/file, format='raw')
   self.vm.launch()
   self.vm.shutdown()

Or even:

   with self.vm as vm:
      vm.add_args('-nodefaults', '-S')
      vm.add_console('isa-serial')
      vm.add_drive(file='/path/to/file, format='raw')
      vm.launch()
   # shutdown() called on __exit__()

Which IMO, are superior in readability and clarity.  This is a matter of
choosing a style, so that we can expect (demand?) that tests follow it.

I'm certainly less experienced with the project dynamics than most here,
so I'm strongly hoping for your input to quickly define the code style
to be used here.

Because of that, I'll skip changing anything here on v4.

- Cleber.