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

[Cleber Rosa]

On 05/30/2018 04:00 PM, Eduardo Habkost wrote:
> On Wed, May 30, 2018 at 02:00:48PM -0400, Cleber Rosa wrote:
>>
>>
>> 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.
> 
> Which new issues?  I don't see any below.
> 

Maybe "issues" is a too strong of a term, but I mean that method
chaining raises question which, IMO, need consideration.  Kind of what
we're doing now.

>>
>> 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.
> 
> Why the "return self" would be a problem?
> 
> 

It bothers *me* personally, but, fair enough, it may not be a problem to
most people.  Now, in addition to this very personal, and thus
insignificant botheration, it restricts API design.  By signaling to
users that this is valid:

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

We'd be restricted on the design of, say, migrate().  Even if it sounds
sensible to return a boolean indicating migration success, for
consistency, it'd require a "return self".

>>
>> 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()
> 
> Maybe it's a matter of personal taste, but I don't see a problem
> with this:
> 
>     self.vm.add_args('-nodefaults', '-S') \
>         .add_console('isa-serial') \
>         .add_drive(file='/path/to/file, format='raw') \
>         .launch() \
>         .shutdown()
> 

Citing PEP-8, backslashes are not the preferred line continuation
mechanism.  They're considerate appropriate only when "implicit
continuation" are not possible.  Still, I don't think QEMU or any other
project must or should follow PEP-8 down to the very last point, so I
consider this mostly a best practice and a small issue.  But, as a side
note, I won't deny that it bothers *me*.

>>
>> Or this:
>>
>>    (self.vm.add_args('-nodefaults', '-S').add_console('isa-serial')
>>            .add_drive(file='/path/to/file, format='raw').launch()
>>            .shutdown())
> 
> If you end up with very complex chains like above, you are still
> free to split it into multiple statements.
> 
> I don't see why this would be an argument against making this
> possible:
> 
>   self.vm.add_args('-nodefaults', '-S').launch()
> 
> 

Right, it's *mostly* style.  But, it suggests that all methods work
similarly and thus restricts API design as mentioned before.

>>
>> 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()
> 
> This is reasonable style if the .add_*() calls don't fit on a
> single line.
> 
> 

Yep.

>>
>> 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__()
> 
> I don't like the extra indentation level, but people are free to
> use this style.
> 
> (BTW, I think avocado_qemu must call shutdown() automatically
> instead of requiring test code to do it manually.)
> 
> 

And it does.  Still, the API is available and there may be tests which
will want to manually shut it down, and then maybe launch it again.  So,
I mentioned it here for completeness.

>>
>> 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.
> 
> They are more readable than the complex method chaining examples
> you have shown, but I don't see any argument against:
> 
>   self.vm.add_args(...).launch()
> 
> 

Right.  This one looks nice indeed.  *My* issues are related to
expectations (all methods support this?), consistency (all methods
should support this!) and the design limitations it brings.

>>
>> 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.
> 
> Agreed: I don't think any changes to the add_args() style need to
> be on v4.  I'd prefer to work to include avocado_qemu first, and
> later consider how we can improve the qemu.py and avocado_qemu.py
> APIs.
> 

Agreed! (Late, v4 is out, but still agreed!)