* [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure
2018-05-29 19:37 [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests Cleber Rosa
@ 2018-05-29 19:37 ` Cleber Rosa
2018-05-30 12:54 ` Stefan Hajnoczi
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments Cleber Rosa
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-29 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Philippe Mathieu-Daudé, Stefan Hajnoczi,
Fam Zheng, Amador Pahim, Cleber Rosa
This patch adds the very minimum infrastructure necessary for writing
and running functional/acceptance tests, including:
* Documentation
* The avocado_qemu.Test base test class
* One example tests (test_version.py)
Additional functionality is expected to be added along the tests that
require them.
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
docs/devel/testing.rst | 153 ++++++++++++++++++++++
tests/acceptance/README.rst | 10 ++
tests/acceptance/avocado_qemu/__init__.py | 54 ++++++++
tests/acceptance/version.py | 24 ++++
4 files changed, 241 insertions(+)
create mode 100644 tests/acceptance/README.rst
create mode 100644 tests/acceptance/avocado_qemu/__init__.py
create mode 100644 tests/acceptance/version.py
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 0ca1a2d4b5..5e090c87fb 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -484,3 +484,156 @@ supported. To start the fuzzer, run
Alternatively, some command different from "qemu-img info" can be tested, by
changing the ``-c`` option.
+
+Acceptance tests using the Avocado Framework
+============================================
+
+The ``tests/acceptance`` directory hosts functional tests, also known
+as acceptance level tests. They're usually higher level, and may
+interact with external resources and with various guest operating
+systems.
+
+These tests are written using the Avocado Testing Framework (which must
+be installed separately) in conjunction with a the ``avocado_qemu.Test``
+class, implemented at ``tests/acceptance/avocado_qemu``.
+
+Installation
+------------
+
+To install Avocado and its dependencies, run:
+
+.. code::
+
+ pip install --user avocado-framework
+
+Alternatively, follow the instructions on this link:
+
+ http://avocado-framework.readthedocs.io/en/latest/GetStartedGuide.html#installing-avocado
+
+Overview
+--------
+
+This directory provides the ``avocado_qemu`` Python module, containing
+the ``avocado_qemu.Test`` class. Here's a simple usage example:
+
+.. code::
+
+ from avocado_qemu import Test
+
+
+ class Version(Test):
+ """
+ :avocado: enable
+ :avocado: tags=quick
+ """
+ def test_qmp_human_info_version(self):
+ self.vm.launch()
+ res = self.vm.command('human-monitor-command',
+ command_line='info version')
+ self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
+
+To execute your test, run:
+
+.. code::
+
+ avocado run version.py
+
+To run all tests in the current directory, tagged in a particular way,
+run:
+
+.. code::
+
+ avocado run -t <TAG> .
+
+The ``avocado_qemu.Test`` base test class
+-----------------------------------------
+
+The ``avocado_qemu.Test`` class has a number of characteristics that
+are worth being mentioned right away.
+
+First of all, it attempts to give each test a ready to use QEMUMachine
+instance, available at ``self.vm``. Because many tests will tweak the
+QEMU command line, launching the QEMUMachine (by using ``self.vm.launch()``)
+is left to the test writer.
+
+At test "tear down", ``avocado_qemu.Test`` handles the QEMUMachine
+shutdown.
+
+QEMUMachine
+~~~~~~~~~~~
+
+The QEMUMachine API should be somewhat familiar to QEMU hackers. It's
+used in the Python iotests, device-crash-test and other Python scripts.
+
+QEMU binary selection
+~~~~~~~~~~~~~~~~~~~~~
+
+The QEMU binary used for the ``self.vm`` QEMUMachine instance will
+primarily depend on the value of the ``qemu_bin`` parameter. If it's
+not explicitly set, its default value will be the result of a dynamic
+probe in the same source tree. A suitable binary will be one that
+targets the architecture matching host machine.
+
+Based on this description, test writers will usually rely on one of
+the following approaches:
+
+1) Set ``qemu_bin``, and use the given binary
+
+2) Do not set ``qemu_bin``, and use a QEMU binary named like
+ "${arch}-softmmu/qemu-system-${arch}", either in the current
+ working directory, or in the current source tree.
+
+The resulting ``qemu_bin`` value will be preserved in the
+``avocado_qemu.Test`` as an attribute with the same name.
+
+Attribute reference
+-------------------
+
+Besides the attributes and methods that are part of the base
+``avocado.Test`` class, the following attributes are available on any
+``avocado_qemu.Test`` instance.
+
+vm
+~~
+
+A QEMUMachine instance, initially configured according to the given
+``qemu_bin`` parameter.
+
+qemu_bin
+~~~~~~~~
+
+The preserved value of the ``qemu_bin`` parameter or the result of the
+dynamic probe for a QEMU binary in the current working directory or
+source tree.
+
+Parameter reference
+-------------------
+
+To understand how Avocado parameters are accessed by tests, and how
+they can be passed to tests, please refer to::
+
+ http://avocado-framework.readthedocs.io/en/latest/WritingTests.html#accessing-test-parameters
+
+Parameter values can be easily seen in the log files, and will look
+like the following:
+
+.. code::
+
+ PARAMS (key=qemu_bin, path=*, default=x86_64-softmmu/qemu-system-x86_64) => 'x86_64-softmmu/qemu-system-x86_64
+
+qemu_bin
+~~~~~~~~
+
+The exact QEMU binary to be used on QEMUMachine.
+
+Uninstalling Avocado
+--------------------
+
+If you've followed the installation instructions above, you can easily
+uninstall Avocado. Start by listing the packages you have installed::
+
+ pip list --user
+
+And remove any package you want with::
+
+ pip uninstall <package_name>
diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
new file mode 100644
index 0000000000..89260faed6
--- /dev/null
+++ b/tests/acceptance/README.rst
@@ -0,0 +1,10 @@
+============================================
+Acceptance tests using the Avocado Framework
+============================================
+
+This directory contains functional tests, also known as acceptance
+level tests. They're usually higher level, and may interact with
+external resources and with various guest operating systems.
+
+For more information, please refer to ``docs/devel/testing.rst``,
+section "Acceptance tests using the Avocado Framework".
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
new file mode 100644
index 0000000000..1e54fd5932
--- /dev/null
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -0,0 +1,54 @@
+# Test class and utilities for functional tests
+#
+# 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.
+
+import os
+import sys
+
+import avocado
+
+SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
+SRC_ROOT_DIR = os.path.abspath(os.path.dirname(SRC_ROOT_DIR))
+sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
+
+from qemu import QEMUMachine
+
+def is_readable_executable_file(path):
+ return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
+
+
+def pick_default_qemu_bin():
+ """
+ Picks the path of a QEMU binary, starting either in the current working
+ directory or in the source tree root directory.
+ """
+ arch = os.uname()[4]
+ qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
+ "qemu-system-%s" % arch)
+ if is_readable_executable_file(qemu_bin_relative_path):
+ return qemu_bin_relative_path
+
+ qemu_bin_from_src_dir_path = os.path.join(SRC_ROOT_DIR,
+ qemu_bin_relative_path)
+ if is_readable_executable_file(qemu_bin_from_src_dir_path):
+ return qemu_bin_from_src_dir_path
+
+
+class Test(avocado.Test):
+ def setUp(self):
+ self.vm = None
+ self.qemu_bin = self.params.get('qemu_bin',
+ default=pick_default_qemu_bin())
+ if self.qemu_bin is None:
+ self.cancel("No QEMU binary defined or found in the source tree")
+ self.vm = QEMUMachine(self.qemu_bin)
+
+ def tearDown(self):
+ if self.vm is not None:
+ self.vm.shutdown()
diff --git a/tests/acceptance/version.py b/tests/acceptance/version.py
new file mode 100644
index 0000000000..13b0a7440d
--- /dev/null
+++ b/tests/acceptance/version.py
@@ -0,0 +1,24 @@
+# Version check example test
+#
+# 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 Version(Test):
+ """
+ :avocado: enable
+ :avocado: tags=quick
+ """
+ def test_qmp_human_info_version(self):
+ self.vm.launch()
+ res = self.vm.command('human-monitor-command',
+ command_line='info version')
+ self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
--
2.17.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure Cleber Rosa
@ 2018-05-30 12:54 ` Stefan Hajnoczi
2018-05-30 18:15 ` Cleber Rosa
0 siblings, 1 reply; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 12:54 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
On Tue, May 29, 2018 at 03:37:26PM -0400, Cleber Rosa wrote:
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 0ca1a2d4b5..5e090c87fb 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -484,3 +484,156 @@ supported. To start the fuzzer, run
>
> Alternatively, some command different from "qemu-img info" can be tested, by
> changing the ``-c`` option.
> +
> +Acceptance tests using the Avocado Framework
> +============================================
> +
> +The ``tests/acceptance`` directory hosts functional tests, also known
> +as acceptance level tests. They're usually higher level, and may
> +interact with external resources and with various guest operating
> +systems.
Can you give more concrete examples of what acceptance tests do? This
will make it easier for readers to understand where Avocado fits in.
Things that come to mind: checking the behavior of QMP commands and
guest OS commands.
> +To run all tests in the current directory, tagged in a particular way,
> +run:
> +
> +.. code::
> +
> + avocado run -t <TAG> .
What is TAG? I guess this identifies a subset of tests to run?
> +QEMUMachine
> +~~~~~~~~~~~
> +
> +The QEMUMachine API should be somewhat familiar to QEMU hackers. It's
> +used in the Python iotests, device-crash-test and other Python scripts.
This is not useful to someone who isn't a QEMU hacker :).
Please explain that QEMUMachine is a class for controlling a QEMU
instance, including command-line arguments and monitor commands/events.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure
2018-05-30 12:54 ` Stefan Hajnoczi
@ 2018-05-30 18:15 ` Cleber Rosa
0 siblings, 0 replies; 20+ messages in thread
From: Cleber Rosa @ 2018-05-30 18:15 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
On 05/30/2018 08:54 AM, Stefan Hajnoczi wrote:
> On Tue, May 29, 2018 at 03:37:26PM -0400, Cleber Rosa wrote:
>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
>> index 0ca1a2d4b5..5e090c87fb 100644
>> --- a/docs/devel/testing.rst
>> +++ b/docs/devel/testing.rst
>> @@ -484,3 +484,156 @@ supported. To start the fuzzer, run
>>
>> Alternatively, some command different from "qemu-img info" can be tested, by
>> changing the ``-c`` option.
>> +
>> +Acceptance tests using the Avocado Framework
>> +============================================
>> +
>> +The ``tests/acceptance`` directory hosts functional tests, also known
>> +as acceptance level tests. They're usually higher level, and may
>> +interact with external resources and with various guest operating
>> +systems.
>
> Can you give more concrete examples of what acceptance tests do? This
> will make it easier for readers to understand where Avocado fits in.
>
> Things that come to mind: checking the behavior of QMP commands and
> guest OS commands.
>
Sure. How about:
---
Tests based on ``avocado_qemu.Test`` can easily:
* Customize the command line arguments given to the convenience
``self.vm`` attribute (a QEMUMachine instance)
* Interact with the QEMU monitor, send QMP commands and check
their results
* Interact with the guest OS, using the convenience console device
(which may be useful to assert the effectiveness and correctness of
command line arguments or QMP commands)
* Interact with external data files that accompany the test itself
(see ``avocado.Test.get_data()``)
* Donwload (and cache) remote data files such as firmware and kernel
images
* Have access to a library of guest OS images (by means of the
``avocado.utils.vmimage`` library)
* Make use of various other test related utilities available at the
test class itself and at the utility library:
-
http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test
-
http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html
---
>> +To run all tests in the current directory, tagged in a particular way,
>> +run:
>> +
>> +.. code::
>> +
>> + avocado run -t <TAG> .
>
> What is TAG? I guess this identifies a subset of tests to run?
>
I'm trying to avoid too many references to the Avocado documentation, so
this could become:
---
Tests may be classified according to a convention by using docstring
directives such as ``:avocado: tags=TAG1,TAG2``. To run all tests
in the current directory, tagged as "quick", run:
.. code::
avocado run -t quick .
---
>> +QEMUMachine
>> +~~~~~~~~~~~
>> +
>> +The QEMUMachine API should be somewhat familiar to QEMU hackers. It's
>> +used in the Python iotests, device-crash-test and other Python scripts.
>
> This is not useful to someone who isn't a QEMU hacker :).
>
True... so I would ask, why are they wandering around here? :D
Now, seriously, I'll add a bit of extra info here on v4.
> Please explain that QEMUMachine is a class for controlling a QEMU
> instance, including command-line arguments and monitor commands/events.
>
Sure thing.
- Cleber.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments
2018-05-29 19:37 [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests Cleber Rosa
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure Cleber Rosa
@ 2018-05-29 19:37 ` Cleber Rosa
2018-05-30 12:54 ` Stefan Hajnoczi
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests Cleber Rosa
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-29 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Philippe Mathieu-Daudé, Stefan Hajnoczi,
Fam Zheng, Amador Pahim, Cleber Rosa
Tests will often need to add extra arguments to QEMU command
line arguments.
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qemu.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 08a3e9af5a..7cd8193df8 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -359,3 +359,9 @@ class QEMUMachine(object):
of the qemu process.
'''
return self._iolog
+
+ def add_args(self, *args):
+ '''
+ Adds to the list of extra arguments to be given to the QEMU binary
+ '''
+ self._args.extend(args)
--
2.17.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments Cleber Rosa
@ 2018-05-30 12:54 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 12:54 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]
On Tue, May 29, 2018 at 03:37:27PM -0400, Cleber Rosa wrote:
> Tests will often need to add extra arguments to QEMU command
> line arguments.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
> scripts/qemu.py | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-29 19:37 [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests Cleber Rosa
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 1/5] Add functional/acceptance tests infrastructure Cleber Rosa
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 2/5] scripts/qemu.py: allow adding to the list of extra arguments Cleber Rosa
@ 2018-05-29 19:37 ` Cleber Rosa
2018-05-30 12:57 ` Stefan Hajnoczi
2018-05-30 12:57 ` Stefan Hajnoczi
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method Cleber Rosa
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test Cleber Rosa
4 siblings, 2 replies; 20+ messages in thread
From: Cleber Rosa @ 2018-05-29 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Philippe Mathieu-Daudé, Stefan Hajnoczi,
Fam Zheng, Amador Pahim, Cleber Rosa
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()
+ 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'], {})
--
2.17.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests Cleber Rosa
@ 2018-05-30 12:57 ` Stefan Hajnoczi
2018-05-30 13:04 ` Fam Zheng
2018-05-30 16:29 ` Eduardo Habkost
2018-05-30 12:57 ` Stefan Hajnoczi
1 sibling, 2 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 12:57 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]
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')
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 12:57 ` Stefan Hajnoczi
@ 2018-05-30 13:04 ` Fam Zheng
2018-05-30 16:29 ` Eduardo Habkost
1 sibling, 0 replies; 20+ messages in thread
From: Fam Zheng @ 2018-05-30 13:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Cleber Rosa, qemu-devel, Eduardo Habkost,
Philippe Mathieu-Daudé, Amador Pahim
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 12:57 ` Stefan Hajnoczi
2018-05-30 13:04 ` Fam Zheng
@ 2018-05-30 16:29 ` Eduardo Habkost
2018-05-30 18:00 ` Cleber Rosa
1 sibling, 1 reply; 20+ messages in thread
From: Eduardo Habkost @ 2018-05-30 16:29 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Cleber Rosa, qemu-devel, Philippe Mathieu-Daudé, Fam Zheng,
Amador Pahim
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()
--
Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 16:29 ` Eduardo Habkost
@ 2018-05-30 18:00 ` Cleber Rosa
2018-05-30 20:00 ` Eduardo Habkost
0 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-30 18:00 UTC (permalink / raw)
To: Eduardo Habkost, Stefan Hajnoczi
Cc: qemu-devel, Philippe Mathieu-Daudé, Fam Zheng, Amador Pahim
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.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 18:00 ` Cleber Rosa
@ 2018-05-30 20:00 ` Eduardo Habkost
2018-05-30 21:03 ` Cleber Rosa
0 siblings, 1 reply; 20+ messages in thread
From: Eduardo Habkost @ 2018-05-30 20:00 UTC (permalink / raw)
To: Cleber Rosa
Cc: Stefan Hajnoczi, qemu-devel, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
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.
--
Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 20:00 ` Eduardo Habkost
@ 2018-05-30 21:03 ` Cleber Rosa
2018-05-30 21:31 ` Eduardo Habkost
0 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-30 21:03 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Amador Pahim, Fam Zheng, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé
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!)
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 21:03 ` Cleber Rosa
@ 2018-05-30 21:31 ` Eduardo Habkost
2018-05-30 22:28 ` Cleber Rosa
0 siblings, 1 reply; 20+ messages in thread
From: Eduardo Habkost @ 2018-05-30 21:31 UTC (permalink / raw)
To: Cleber Rosa
Cc: Amador Pahim, Fam Zheng, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé
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()
--
Eduardo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-30 21:31 ` Eduardo Habkost
@ 2018-05-30 22:28 ` Cleber Rosa
0 siblings, 0 replies; 20+ messages in thread
From: Cleber Rosa @ 2018-05-30 22:28 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Amador Pahim, Fam Zheng, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests Cleber Rosa
2018-05-30 12:57 ` Stefan Hajnoczi
@ 2018-05-30 12:57 ` Stefan Hajnoczi
1 sibling, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 12:57 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 391 bytes --]
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>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method
2018-05-29 19:37 [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests Cleber Rosa
` (2 preceding siblings ...)
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests Cleber Rosa
@ 2018-05-29 19:37 ` Cleber Rosa
2018-05-30 19:17 ` Stefan Hajnoczi
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test Cleber Rosa
4 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-29 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Philippe Mathieu-Daudé, Stefan Hajnoczi,
Fam Zheng, Amador Pahim, Cleber Rosa
The set_console() method is intended to ease higher level use cases
that require a console device.
The amount of intelligence is limited on purpose, requiring either the
device type explicitly, or the existence of a machine (pattern)
definition.
Because of the console device type selection criteria (by machine
type), users should also be able to define that. It'll then be used
for both '-machine' and for the console device type selection.
Users of the set_console() method will certainly be interested in
accessing the console device, and for that a console_socket property
has been added.
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qemu.py | 97 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 96 insertions(+), 1 deletion(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7cd8193df8..f099ce7278 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -17,19 +17,41 @@ import logging
import os
import subprocess
import qmp.qmp
+import re
import shutil
+import socket
import tempfile
LOG = logging.getLogger(__name__)
+#: Maps machine types to the preferred console device types
+CONSOLE_DEV_TYPES = {
+ r'^clipper$': 'isa-serial',
+ r'^malta': 'isa-serial',
+ r'^(pc.*|q35.*|isapc)$': 'isa-serial',
+ r'^(40p|powernv|prep)$': 'isa-serial',
+ r'^pseries.*': 'spapr-vty',
+ r'^s390-ccw-virtio.*': 'sclpconsole',
+ }
+
+
class QEMUMachineError(Exception):
"""
Exception called when an error in QEMUMachine happens.
"""
+class QEMUMachineAddDeviceError(QEMUMachineError):
+ """
+ Exception raised when a request to add a device can not be fulfilled
+
+ The failures are caused by limitations, lack of information or conflicting
+ requests on the QEMUMachine methods. This exception does not represent
+ failures reported by the QEMU binary itself.
+ """
+
class MonitorResponseError(qmp.qmp.QMPError):
'''
Represents erroneous QMP monitor reply
@@ -91,6 +113,10 @@ class QEMUMachine(object):
self._test_dir = test_dir
self._temp_dir = None
self._launched = False
+ self._machine = None
+ self._console_device_type = None
+ self._console_address = None
+ self._console_socket = None
# just in case logging wasn't configured by the main script:
logging.basicConfig()
@@ -175,9 +201,19 @@ class QEMUMachine(object):
self._monitor_address[1])
else:
moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
- return ['-chardev', moncdev,
+ args = ['-chardev', moncdev,
'-mon', 'chardev=mon,mode=control',
'-display', 'none', '-vga', 'none']
+ if self._machine is not None:
+ args.extend(['-machine', self._machine])
+ if self._console_device_type is not None:
+ self._console_address = os.path.join(self._temp_dir,
+ self._name + "-console.sock")
+ chardev = ('socket,id=console,path=%s,server,nowait' %
+ self._console_address)
+ device = '%s,chardev=console' % self._console_device_type
+ args.extend(['-chardev', chardev, '-device', device])
+ return args
def _pre_launch(self):
self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
@@ -202,6 +238,10 @@ class QEMUMachine(object):
self._qemu_log_path = None
+ if self._console_socket is not None:
+ self._console_socket.close()
+ self._console_socket = None
+
if self._temp_dir is not None:
shutil.rmtree(self._temp_dir)
self._temp_dir = None
@@ -365,3 +405,58 @@ class QEMUMachine(object):
Adds to the list of extra arguments to be given to the QEMU binary
'''
self._args.extend(args)
+
+ def set_machine(self, machine_type):
+ '''
+ Sets the machine type
+
+ If set, the machine type will be added to the base arguments
+ of the resulting QEMU command line.
+ '''
+ self._machine = machine_type
+
+ def set_console(self, device_type=None):
+ '''
+ Sets the device type for a console device
+
+ If set, the console device and a backing character device will
+ be added to the base arguments of the resulting QEMU command
+ line.
+
+ This is a convenience method that will either use the provided
+ device type, of if not given, it will used the device type set
+ on CONSOLE_DEV_TYPES.
+
+ The actual setting of command line arguments will be be done at
+ machine launch time, as it depends on the temporary directory
+ to be created.
+
+ @param device_type: the device type, such as "isa-serial"
+ @raises: QEMUMachineAddDeviceError if the device type is not given
+ and can not be determined.
+ '''
+ if device_type is None:
+ if self._machine is None:
+ raise QEMUMachineAddDeviceError("Can not add a console device:"
+ " QEMU instance without a "
+ "defined machine type")
+ for regex, device in CONSOLE_DEV_TYPES.items():
+ if re.match(regex, self._machine):
+ device_type = device
+ break
+ if device_type is None:
+ raise QEMUMachineAddDeviceError("Can not add a console device:"
+ " no matching console device "
+ "type definition")
+ self._console_device_type = device_type
+
+ @property
+ def console_socket(self):
+ """
+ Returns a socket connected to the console
+ """
+ if self._console_socket is None:
+ self._console_socket = socket.socket(socket.AF_UNIX,
+ socket.SOCK_STREAM)
+ self._console_socket.connect(self._console_address)
+ return self._console_socket
--
2.17.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method Cleber Rosa
@ 2018-05-30 19:17 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 19:17 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 943 bytes --]
On Tue, May 29, 2018 at 03:37:29PM -0400, Cleber Rosa wrote:
> The set_console() method is intended to ease higher level use cases
> that require a console device.
>
> The amount of intelligence is limited on purpose, requiring either the
> device type explicitly, or the existence of a machine (pattern)
> definition.
>
> Because of the console device type selection criteria (by machine
> type), users should also be able to define that. It'll then be used
> for both '-machine' and for the console device type selection.
>
> Users of the set_console() method will certainly be interested in
> accessing the console device, and for that a console_socket property
> has been added.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
> scripts/qemu.py | 97 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 96 insertions(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test
2018-05-29 19:37 [Qemu-devel] [PATCH v3 0/5] Acceptance/functional tests Cleber Rosa
` (3 preceding siblings ...)
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 4/5] scripts/qemu.py: introduce set_console() method Cleber Rosa
@ 2018-05-29 19:37 ` Cleber Rosa
2018-05-30 19:20 ` Stefan Hajnoczi
4 siblings, 1 reply; 20+ messages in thread
From: Cleber Rosa @ 2018-05-29 19:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Philippe Mathieu-Daudé, Stefan Hajnoczi,
Fam Zheng, Amador Pahim, Cleber Rosa
This test boots a Linux kernel, and checks that the given command
line was effective in two ways:
* It makes the kernel use the set "console device" as a console
* The kernel records the command line as expected in the console
Given that way too many error conditions may occur, and detecting the
kernel boot progress status may not be trivial, this test relies on a
timeout to handle unexpected situations. Also, it's *not* tagged as a
quick test for obvious reasons.
It may be useful, while interactively running/debugging this test, or
tests similar to this one, to show some of the logging channels.
Example:
$ avocado --show=QMP,console run boot_linux_console.py
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
tests/acceptance/boot_linux_console.py | 47 ++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 tests/acceptance/boot_linux_console.py
diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
new file mode 100644
index 0000000000..98324f7591
--- /dev/null
+++ b/tests/acceptance/boot_linux_console.py
@@ -0,0 +1,47 @@
+# Functional test that boots a Linux kernel and checks the console
+#
+# 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.
+
+import logging
+
+from avocado_qemu import Test
+
+
+class BootLinuxConsole(Test):
+ """
+ Boots a x86_64 Linux kernel and checks that the console is operational
+ and the kernel command line is properly passed from QEMU to the kernel
+
+ :avocado: enable
+ :avocado: tags=x86_64
+ """
+
+ timeout = 60
+
+ def test(self):
+ kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
+ 'Everything/x86_64/os/images/pxeboot/vmlinuz')
+ kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
+ kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+ self.vm.set_machine('pc')
+ self.vm.set_console()
+ kernel_command_line = 'console=ttyS0'
+ self.vm.add_args('-kernel', kernel_path,
+ '-append', kernel_command_line)
+ self.vm.launch()
+ console = self.vm.console_socket.makefile()
+ console_logger = logging.getLogger('console')
+ while True:
+ msg = console.readline()
+ console_logger.debug(msg.strip())
+ if 'Kernel command line: %s' % kernel_command_line in msg:
+ break
+ if 'Kernel panic - not syncing' in msg:
+ self.fail("Kernel panic reached")
--
2.17.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test
2018-05-29 19:37 ` [Qemu-devel] [PATCH v3 5/5] Acceptance tests: add Linux kernel boot and console checking test Cleber Rosa
@ 2018-05-30 19:20 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2018-05-30 19:20 UTC (permalink / raw)
To: Cleber Rosa
Cc: qemu-devel, Eduardo Habkost, Philippe Mathieu-Daudé,
Fam Zheng, Amador Pahim
[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]
On Tue, May 29, 2018 at 03:37:30PM -0400, Cleber Rosa wrote:
> This test boots a Linux kernel, and checks that the given command
> line was effective in two ways:
>
> * It makes the kernel use the set "console device" as a console
> * The kernel records the command line as expected in the console
>
> Given that way too many error conditions may occur, and detecting the
> kernel boot progress status may not be trivial, this test relies on a
> timeout to handle unexpected situations. Also, it's *not* tagged as a
> quick test for obvious reasons.
>
> It may be useful, while interactively running/debugging this test, or
> tests similar to this one, to show some of the logging channels.
> Example:
>
> $ avocado --show=QMP,console run boot_linux_console.py
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
> tests/acceptance/boot_linux_console.py | 47 ++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 tests/acceptance/boot_linux_console.py
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread