[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests |
Date: |
Wed, 30 May 2018 17:00:32 -0300 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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
- [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests, Cleber Rosa, 2018/05/29
- [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Cleber Rosa, 2018/05/29
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Stefan Hajnoczi, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Fam Zheng, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Eduardo Habkost, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Cleber Rosa, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Cleber Rosa, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Eduardo Habkost, 2018/05/30
- Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Cleber Rosa, 2018/05/30
Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests, Stefan Hajnoczi, 2018/05/30
[Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments, Cleber Rosa, 2018/05/29
[Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test, Cleber Rosa, 2018/05/29
[Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method, Cleber Rosa, 2018/05/29