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 <cr...@redhat.com>
>>>>> ---
>>>>> 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 <cr...@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 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!)
