diff mbox

[v3,3/5] Acceptance tests: add quick VNC tests

Message ID 20180529193730.9204-4-crosa@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cleber Rosa May 29, 2018, 7:37 p.m. UTC
This patch adds a few simple behavior tests for VNC.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/vnc.py | 60 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 tests/acceptance/vnc.py

Comments

Stefan Hajnoczi May 30, 2018, 12:57 p.m. UTC | #1
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 <crosa@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 <crosa@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')
Stefan Hajnoczi May 30, 2018, 12:57 p.m. UTC | #2
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 <crosa@redhat.com>
> ---
>  tests/acceptance/vnc.py | 60 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 tests/acceptance/vnc.py

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Fam Zheng May 30, 2018, 1:04 p.m. UTC | #3
On Wed, 05/30 13:57, 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 <crosa@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 <crosa@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')

+1. It will be a nice extension to the launch method.

Fam
Eduardo Habkost May 30, 2018, 4:29 p.m. UTC | #4
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 <crosa@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 <crosa@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()
Cleber Rosa May 30, 2018, 6 p.m. UTC | #5
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 <crosa@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 <crosa@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.

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.
Eduardo Habkost May 30, 2018, 8 p.m. UTC | #6
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 <crosa@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 <crosa@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.

> 
> 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.
Cleber Rosa May 30, 2018, 9:03 p.m. UTC | #7
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 <crosa@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 <crosa@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!)
Eduardo Habkost May 30, 2018, 9:31 p.m. UTC | #8
On Wed, May 30, 2018 at 05:03:56PM -0400, Cleber Rosa wrote:
> On 05/30/2018 04:00 PM, Eduardo Habkost wrote:
[...]
>       [...]   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".
[...]
> > 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.
[...]
> > 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.
> 

These are good points.

I believe it would be a reasonable convention to make methods
that change QEMU configuration return self (because they are
often called one after another), but other methods like launch()
migrate(), and shutdown() don't have to.

So this would work:

  self.vm.add_args(...).add_drive(...).launch()

But this doesn't have to:

  self.vm.add_args(...).launch().migrate().shutdown()
Cleber Rosa May 30, 2018, 10:28 p.m. UTC | #9
On 05/30/2018 05:31 PM, Eduardo Habkost wrote:
> On Wed, May 30, 2018 at 05:03:56PM -0400, Cleber Rosa wrote:
>> On 05/30/2018 04:00 PM, Eduardo Habkost wrote:
> [...]
>>       [...]   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".
> [...]
>>> 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.
> [...]
>>> 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.
>>
> 
> These are good points.
> 
> I believe it would be a reasonable convention to make methods
> that change QEMU configuration return self (because they are
> often called one after another), but other methods like launch()
> migrate(), and shutdown() don't have to.
> 
> So this would work:
> 
>   self.vm.add_args(...).add_drive(...).launch()
> 
> But this doesn't have to:
> 
>   self.vm.add_args(...).launch().migrate().shutdown()
> 

Looks like a sane general rule.  Let's revisit this when new add_*()
methods are added.

- Cleber.
diff mbox

Patch

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 <crosa@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()
+        self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
+
+    def test_no_vnc_change_password(self):
+        self.vm.add_args('-nodefaults', '-S')
+        self.vm.launch()
+        self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
+        set_password_response = self.vm.qmp('change',
+                                            device='vnc',
+                                            target='password',
+                                            arg='new_password')
+        self.assertIn('error', set_password_response)
+        self.assertEqual(set_password_response['error']['class'],
+                         'GenericError')
+        self.assertEqual(set_password_response['error']['desc'],
+                         'Could not set password')
+
+    def test_vnc_change_password_requires_a_password(self):
+        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
+        self.vm.launch()
+        self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
+        set_password_response = self.vm.qmp('change',
+                                            device='vnc',
+                                            target='password',
+                                            arg='new_password')
+        self.assertIn('error', set_password_response)
+        self.assertEqual(set_password_response['error']['class'],
+                         'GenericError')
+        self.assertEqual(set_password_response['error']['desc'],
+                         'Could not set password')
+
+    def test_vnc_change_password(self):
+        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password')
+        self.vm.launch()
+        self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
+        set_password_response = self.vm.qmp('change',
+                                            device='vnc',
+                                            target='password',
+                                            arg='new_password')
+        self.assertEqual(set_password_response['return'], {})