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

[Eduardo Habkost]

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.

> 
> 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?


> 
> 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()

> 
> 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()


> 
> 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.


> 
> 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.)


> 
> 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()


> 
> 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.

-- 
Eduardo