[Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Cleber Rosa]

TL;DR
=====

Another version, with a minimalist approach, to the acceptance tests
infrastructure for QEMU, based on the Avocado Testing Framework.

Background
==========

The previous version, still considered an RFC, was sent to the list by
Eduardo[1] was based on the work held in Amador's branch[2].  After
reviewing in under a different light, including the experiences
done and reported by Philippe[3].

Differences from previous versions
==================================

The main difference is that this series include only the minimal
changes deemed necessary to have a starting point.  I like to think
that it's better connected to the QEMU community and project needs,
and will hopefully allow for a more organic growth.

Since this version has less features than the previous versions,
provided it's accepted, these are the next probable development tasks:

 * Provide a simple variants mechanism to allow the same tests to be
   run under different targets, machine models and devices (present on
   the previous versions as a "YAML to Mux" file with architecture
   definitions)
 * Implement QEMUMachine migration support (present on the previous
   version in the "avocado_qemu.test._VM" class)
 * Implement Guest OS image selection and download (mostly an Avocado
   feature, paired with a parameter convention and cloud-init support
   code)
 * Implement interactive support for Guest OS sessions (present on
   the previous versions, supported by the aexpect Python module)

Even though this version shares very little (if any) code with the
previous versions, the following is a list of noteworthy changes:

 * Tests directory is now "tests/acceptance" (was "tests/avocado")
 * Base test class is now "avocado_qemu.Test" (was
   "avocado_qemu.test.QemuTest")
 * Base test class is now hosted in "avocado_qemu/__init__.py" (was
   "avocado_qemu/test.py")
 * Direct use of "qemu.QEMUMachine", that is, the
   avocado_qemu.test._VM class is gone
 * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
   QEMU binary on a custom system location it's necessary to use the
   "qemu_bin" parameter
 * Example test in README.rst is distributed as a real test
   ("test_version.py")
 * A new "Linux boot console" test, loosely modeled after Phillipe's
   use case

Commit summary
==============

Cleber Rosa (5):
  Add functional/acceptance tests infrastructure
  scripts/qemu.py: allow adding to the list of extra arguments
  Acceptance tests: add quick VNC tests
  scripts/qemu.py: introduce set_console() method
  Acceptance tests: add Linux kernel boot and console checking test

 scripts/qemu.py                             | 103 +++++++++++++++-
 scripts/test_qemu.py                        | 176 +++++++++++++++++++++++++++
 tests/acceptance/README.rst                 | 141 +++++++++++++++++++++
 tests/acceptance/avocado_qemu/__init__.py   |  45 +++++++
 tests/acceptance/test_boot_linux_console.py |  37 ++++++
 tests/acceptance/test_version.py            |  13 ++
 tests/acceptance/test_vnc.py                |  50 ++++++++
 7 files changed, 564 insertions(+), 1 deletion(-)
 create mode 100644 scripts/test_qemu.py
 create mode 100644 tests/acceptance/README.rst
 create mode 100644 tests/acceptance/avocado_qemu/__init__.py
 create mode 100644 tests/acceptance/test_boot_linux_console.py
 create mode 100644 tests/acceptance/test_version.py
 create mode 100644 tests/acceptance/test_vnc.py

---

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
[2] https://github.com/apahim/qemu/commits/avocado_qemu
[3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html

[Qemu-devel] [PATCH 1/5] Add functional/acceptance tests infrastructure

[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>
---
 tests/acceptance/README.rst               | 141 ++++++++++++++++++++++
 tests/acceptance/avocado_qemu/__init__.py |  45 +++++++
 tests/acceptance/test_version.py          |  13 ++
 3 files changed, 199 insertions(+)
 create mode 100644 tests/acceptance/README.rst
 create mode 100644 tests/acceptance/avocado_qemu/__init__.py
 create mode 100644 tests/acceptance/test_version.py

diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
new file mode 100644
index 0000000000..f1434177da
--- /dev/null
+++ b/tests/acceptance/README.rst
@@ -0,0 +1,141 @@
+==============================================
+ Acceptance tests using the Avocado Framework
+==============================================
+
+This 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.
+
+The tests are written using the Avocado Testing Framework, in
+conjunction with a the ``avocado_qemu.Test`` class, distributed here.
+
+Installation
+============
+
+To install Avocado and its dependencies, run::
+
+  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::
+
+  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::
+
+  avocado run test_version.py
+
+To run all tests in the current directory, tagged in a particular way,
+run::
+
+  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::
+
+  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/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
new file mode 100644
index 0000000000..dcd58047a3
--- /dev/null
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -0,0 +1,45 @@
+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 = 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/test_version.py b/tests/acceptance/test_version.py
new file mode 100644
index 0000000000..e397f07d83
--- /dev/null
+++ b/tests/acceptance/test_version.py
@@ -0,0 +1,13 @@
+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

[Qemu-devel] [PATCH 2/5] scripts/qemu.py: allow adding to the list of extra arguments

[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..7813dd45ad 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
+        '''
+        return self._args.extend(args)
-- 
2.17.0

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

[Cleber Rosa]

This patch adds a few simple behavior tests for VNC.  These tests
introduce manipulation of the QEMUMachine arguments, by setting
the arguments, instead of adding to the existing ones.

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

diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
new file mode 100644
index 0000000000..9d9a35cf55
--- /dev/null
+++ b/tests/acceptance/test_vnc.py
@@ -0,0 +1,50 @@
+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

[Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Cleber Rosa]

The set_console() method is intended to ease higher level use cases
that require a console device.

The amount of inteligence 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 +++++++++++++++++++++++-
 scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 scripts/test_qemu.py

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7813dd45ad..89db5bc6b2 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
         '''
         return 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
diff --git a/scripts/test_qemu.py b/scripts/test_qemu.py
new file mode 100644
index 0000000000..5e016d3751
--- /dev/null
+++ b/scripts/test_qemu.py
@@ -0,0 +1,176 @@
+import sys
+import os
+import glob
+import unittest
+import shutil
+import tempfile
+import subprocess
+
+import qemu
+import qmp.qmp
+
+
+class QEMUMachineProbeError(Exception):
+    """
+    Exception raised when a probe a fails to be deterministic
+    """
+
+
+def get_built_qemu_binaries(root_dir=None):
+    """
+    Attempts to find QEMU binaries in a tree
+
+    If root_dir is None, it will attempt to find the binaries at the
+    source tree.  It's possible to override it by setting the environment
+    variable QEMU_ROOT_DIR.
+    """
+    if root_dir is None:
+        src_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+        root_dir = os.environ.get("QEMU_ROOT_DIR", src_dir)
+    binaries = glob.glob(os.path.join(root_dir, '*-softmmu/qemu-system-*'))
+    if 'win' in sys.platform:
+        bin_filter = lambda x: x.endswith(".exe")
+    else:
+        bin_filter = lambda x: not x.endswith(".exe")
+    return [_ for _ in binaries if bin_filter(_)]
+
+
+def subprocess_dev_null(mode='w'):
+    """
+    A portable null file object suitable for use with the subprocess module
+    """
+    if hasattr(subprocess, 'DEVNULL'):
+        return subprocess.DEVNULL
+    else:
+        return open(os.path.devnull, mode)
+
+
+def qmp_execute(binary_path, qmp_command):
+    """
+    Executes a QMP command on a given QEMU binary
+
+    Useful for one-off execution of QEMU binaries to get runtime
+    information.
+
+    @param binary_path: path to a QEMU binary
+    @param qmp_command: the QMP command
+    @note: passing arguments to the QMP command is not supported at
+           this time.
+    """
+    try:
+        tempdir = tempfile.mkdtemp()
+        monitor_socket = os.path.join(tempdir, 'monitor.sock')
+        args = [binary_path, '-nodefaults', '-machine', 'none',
+                '-nographic', '-S', '-qmp', 'unix:%s' % monitor_socket]
+        monitor = qmp.qmp.QEMUMonitorProtocol(monitor_socket, True)
+        try:
+            qemu_proc = subprocess.Popen(args,
+                                         stdin=subprocess.PIPE,
+                                         stdout=subprocess.PIPE,
+                                         stderr=subprocess_dev_null(),
+                                         universal_newlines=True)
+        except OSError:
+            return None
+        monitor.accept()
+        res = monitor.cmd(qmp_command)
+        monitor.cmd("quit")
+        qemu_proc.wait()
+        monitor.close()
+        return res.get("return", None)
+    finally:
+        shutil.rmtree(tempdir)
+
+
+def qemu_bin_probe_arch(binary_path):
+    """
+    Probes the architecture from the QEMU binary
+
+    @returns: either the probed arch or None
+    @rtype: str or None
+    @raises: QEMUMachineProbeError
+    """
+    res = qmp_execute(binary_path, "query-target")
+    if res is None:
+        raise QEMUMachineProbeError('Failed to probe the QEMU arch by querying'
+                                    ' the target of binary "%s"' % binary_path)
+    return res.get("arch", None)
+
+
+class QEMU(unittest.TestCase):
+
+
+    TEST_ARCH_MACHINE_CONSOLES = {
+        'alpha': ['clipper'],
+        'mips': ['malta'],
+        'x86_64': ['isapc',
+                   'pc', 'pc-0.10', 'pc-0.11', 'pc-0.12', 'pc-0.13',
+                   'pc-0.14', 'pc-0.15', 'pc-1.0', 'pc-1.1', 'pc-1.2',
+                   'pc-1.3',
+                   'pc-i440fx-1.4', 'pc-i440fx-1.5', 'pc-i440fx-1.6',
+                   'pc-i440fx-1.7', 'pc-i440fx-2.0', 'pc-i440fx-2.1',
+                   'pc-i440fx-2.10', 'pc-i440fx-2.11', 'pc-i440fx-2.2',
+                   'pc-i440fx-2.3', 'pc-i440fx-2.4', 'pc-i440fx-2.5',
+                   'pc-i440fx-2.6', 'pc-i440fx-2.7', 'pc-i440fx-2.8',
+                   'pc-i440fx-2.9', 'pc-q35-2.10', 'pc-q35-2.11',
+                   'q35', 'pc-q35-2.4', 'pc-q35-2.5', 'pc-q35-2.6',
+                   'pc-q35-2.7', 'pc-q35-2.8', 'pc-q35-2.9'],
+        'ppc64': ['40p', 'powernv', 'prep', 'pseries', 'pseries-2.1',
+                  'pseries-2.2', 'pseries-2.3', 'pseries-2.4', 'pseries-2.5',
+                  'pseries-2.6', 'pseries-2.7', 'pseries-2.8', 'pseries-2.9',
+                  'pseries-2.10', 'pseries-2.11', 'pseries-2.12'],
+        's390x': ['s390-ccw-virtio', 's390-ccw-virtio-2.4',
+                  's390-ccw-virtio-2.5', 's390-ccw-virtio-2.6',
+                  's390-ccw-virtio-2.7', 's390-ccw-virtio-2.8',
+                  's390-ccw-virtio-2.9', 's390-ccw-virtio-2.10',
+                  's390-ccw-virtio-2.11', 's390-ccw-virtio-2.12']
+    }
+
+
+    def test_set_console(self):
+        for machines in QEMU.TEST_ARCH_MACHINE_CONSOLES.values():
+            for machine in machines:
+                qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
+                qemu_machine.set_machine(machine)
+                qemu_machine.set_console()
+
+    def test_set_console_no_machine(self):
+        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
+        self.assertRaises(qemu.QEMUMachineAddDeviceError,
+                          qemu_machine.set_console)
+
+    def test_set_console_no_machine_match(self):
+        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
+        qemu_machine.set_machine('unknown-machine-model')
+        self.assertRaises(qemu.QEMUMachineAddDeviceError,
+                          qemu_machine.set_console)
+
+    @unittest.skipUnless(get_built_qemu_binaries(),
+                         "Could not find any QEMU binaries built to use on "
+                         "console check")
+    def test_set_console_launch(self):
+        for binary in get_built_qemu_binaries():
+            probed_arch = qemu_bin_probe_arch(binary)
+            for machine in QEMU.TEST_ARCH_MACHINE_CONSOLES.get(probed_arch, []):
+                qemu_machine = qemu.QEMUMachine(binary)
+
+                # the following workarounds are target specific required for
+                # this test.  users are of QEMUMachine are expected to deal with
+                # target specific requirements just the same in their own code
+                cap_htm_off = ('pseries-2.7', 'pseries-2.8', 'pseries-2.9',
+                               'pseries-2.10', 'pseries-2.11', 'pseries-2.12')
+                if probed_arch == 'ppc64' and machine in cap_htm_off:
+                    qemu_machine._machine = machine   # pylint: disable=W0212
+                    qemu_machine.args.extend(['-machine',
+                                              '%s,cap-htm=off' % machine])
+                elif probed_arch == 's390x':
+                    qemu_machine.set_machine(machine)
+                    qemu_machine.args.append('-nodefaults')
+                elif probed_arch == 'mips':
+                    qemu_machine.set_machine(machine)
+                    qemu_machine.args.extend(['-bios', os.path.devnull])
+                else:
+                    qemu_machine.set_machine(machine)
+
+                qemu_machine.set_console()
+                qemu_machine.launch()
+                qemu_machine.shutdown()
-- 
2.17.0

[Qemu-devel] [PATCH 5/5] Acceptance tests: add Linux kernel boot and console checking test

[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 test_boot_linux_console.py

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

diff --git a/tests/acceptance/test_boot_linux_console.py b/tests/acceptance/test_boot_linux_console.py
new file mode 100644
index 0000000000..52811bfe06
--- /dev/null
+++ b/tests/acceptance/test_boot_linux_console.py
@@ -0,0 +1,37 @@
+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

Re: [Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Cleber Rosa]

On 05/24/2018 08:58 PM, Cleber Rosa wrote:
> TL;DR
> =====
> 
> Another version, with a minimalist approach, to the acceptance tests
> infrastructure for QEMU, based on the Avocado Testing Framework.
> 
> Background
> ==========
> 
> The previous version, still considered an RFC, was sent to the list by
> Eduardo[1] was based on the work held in Amador's branch[2].  After
> reviewing in under a different light, including the experiences
> done and reported by Philippe[3].
> 

(major sigh for killing a line, writing a non-sense sentence)

... it was clear to me that a different approach would be better.

- Cleber.

> Differences from previous versions
> ==================================
> 
> The main difference is that this series include only the minimal
> changes deemed necessary to have a starting point.  I like to think
> that it's better connected to the QEMU community and project needs,
> and will hopefully allow for a more organic growth.
> 
> Since this version has less features than the previous versions,
> provided it's accepted, these are the next probable development tasks:
> 
>  * Provide a simple variants mechanism to allow the same tests to be
>    run under different targets, machine models and devices (present on
>    the previous versions as a "YAML to Mux" file with architecture
>    definitions)
>  * Implement QEMUMachine migration support (present on the previous
>    version in the "avocado_qemu.test._VM" class)
>  * Implement Guest OS image selection and download (mostly an Avocado
>    feature, paired with a parameter convention and cloud-init support
>    code)
>  * Implement interactive support for Guest OS sessions (present on
>    the previous versions, supported by the aexpect Python module)
> 
> Even though this version shares very little (if any) code with the
> previous versions, the following is a list of noteworthy changes:
> 
>  * Tests directory is now "tests/acceptance" (was "tests/avocado")
>  * Base test class is now "avocado_qemu.Test" (was
>    "avocado_qemu.test.QemuTest")
>  * Base test class is now hosted in "avocado_qemu/__init__.py" (was
>    "avocado_qemu/test.py")
>  * Direct use of "qemu.QEMUMachine", that is, the
>    avocado_qemu.test._VM class is gone
>  * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
>    QEMU binary on a custom system location it's necessary to use the
>    "qemu_bin" parameter
>  * Example test in README.rst is distributed as a real test
>    ("test_version.py")
>  * A new "Linux boot console" test, loosely modeled after Phillipe's
>    use case
> 
> Commit summary
> ==============
> 
> Cleber Rosa (5):
>   Add functional/acceptance tests infrastructure
>   scripts/qemu.py: allow adding to the list of extra arguments
>   Acceptance tests: add quick VNC tests
>   scripts/qemu.py: introduce set_console() method
>   Acceptance tests: add Linux kernel boot and console checking test
> 
>  scripts/qemu.py                             | 103 +++++++++++++++-
>  scripts/test_qemu.py                        | 176 +++++++++++++++++++++++++++
>  tests/acceptance/README.rst                 | 141 +++++++++++++++++++++
>  tests/acceptance/avocado_qemu/__init__.py   |  45 +++++++
>  tests/acceptance/test_boot_linux_console.py |  37 ++++++
>  tests/acceptance/test_version.py            |  13 ++
>  tests/acceptance/test_vnc.py                |  50 ++++++++
>  7 files changed, 564 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
>  create mode 100644 tests/acceptance/README.rst
>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>  create mode 100644 tests/acceptance/test_boot_linux_console.py
>  create mode 100644 tests/acceptance/test_version.py
>  create mode 100644 tests/acceptance/test_vnc.py
> 
> ---
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
> [2] https://github.com/apahim/qemu/commits/avocado_qemu
> [3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html
> 
> 

Re: [Qemu-devel] [PATCH 1/5] Add functional/acceptance tests infrastructure

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> 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>
> ---
>  tests/acceptance/README.rst               | 141 ++++++++++++++++++++++

Could you add the actual doc text to docs/devel/testing.rst and reference it
here?

>  tests/acceptance/avocado_qemu/__init__.py |  45 +++++++
>  tests/acceptance/test_version.py          |  13 ++
>  3 files changed, 199 insertions(+)
>  create mode 100644 tests/acceptance/README.rst
>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>  create mode 100644 tests/acceptance/test_version.py
> 
> diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
> new file mode 100644
> index 0000000000..f1434177da
> --- /dev/null
> +++ b/tests/acceptance/README.rst
> @@ -0,0 +1,141 @@
> +==============================================
> + Acceptance tests using the Avocado Framework
> +==============================================
> +
> +This 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.
> +
> +The tests are written using the Avocado Testing Framework, in
> +conjunction with a the ``avocado_qemu.Test`` class, distributed here.
> +
> +Installation
> +============
> +
> +To install Avocado and its dependencies, run::
> +
> +  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::
> +
> +  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::
> +
> +  avocado run test_version.py
> +
> +To run all tests in the current directory, tagged in a particular way,
> +run::
> +
> +  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::
> +
> +  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/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> new file mode 100644
> index 0000000000..dcd58047a3
> --- /dev/null
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -0,0 +1,45 @@

A copy right header is necessary here.

> +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 = 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/test_version.py b/tests/acceptance/test_version.py
> new file mode 100644
> index 0000000000..e397f07d83
> --- /dev/null
> +++ b/tests/acceptance/test_version.py
> @@ -0,0 +1,13 @@
> +from avocado_qemu import Test

Same here.

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

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

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> This patch adds a few simple behavior tests for VNC.  These tests
> introduce manipulation of the QEMUMachine arguments, by setting
> the arguments, instead of adding to the existing ones.

I'm confused by this. The code uses 'add_args', so it does add to the arguments,
no?

> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 tests/acceptance/test_vnc.py
> 
> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
> new file mode 100644
> index 0000000000..9d9a35cf55
> --- /dev/null
> +++ b/tests/acceptance/test_vnc.py
> @@ -0,0 +1,50 @@

Copyright header is missing here too.

Fam

> +from avocado_qemu import Test
> +
> +
> +class Vnc(Test):

Should VncTest be a better class name?

> +    """
> +    :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
> 

Fam

Re: [Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> The set_console() method is intended to ease higher level use cases
> that require a console device.
> 
> The amount of inteligence 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 +++++++++++++++++++++++-
>  scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 272 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7813dd45ad..89db5bc6b2 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
>          '''
>          return 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
> diff --git a/scripts/test_qemu.py b/scripts/test_qemu.py
> new file mode 100644
> index 0000000000..5e016d3751
> --- /dev/null
> +++ b/scripts/test_qemu.py
> @@ -0,0 +1,176 @@

Again, a file header is missing.

> +import sys
> +import os
> +import glob
> +import unittest
> +import shutil
> +import tempfile
> +import subprocess
> +
> +import qemu
> +import qmp.qmp
> +
> +
> +class QEMUMachineProbeError(Exception):
> +    """
> +    Exception raised when a probe a fails to be deterministic
> +    """
> +
> +
> +def get_built_qemu_binaries(root_dir=None):
> +    """
> +    Attempts to find QEMU binaries in a tree
> +
> +    If root_dir is None, it will attempt to find the binaries at the
> +    source tree.  It's possible to override it by setting the environment
> +    variable QEMU_ROOT_DIR.
> +    """
> +    if root_dir is None:
> +        src_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
> +        root_dir = os.environ.get("QEMU_ROOT_DIR", src_dir)
> +    binaries = glob.glob(os.path.join(root_dir, '*-softmmu/qemu-system-*'))
> +    if 'win' in sys.platform:
> +        bin_filter = lambda x: x.endswith(".exe")
> +    else:
> +        bin_filter = lambda x: not x.endswith(".exe")
> +    return [_ for _ in binaries if bin_filter(_)]
> +
> +
> +def subprocess_dev_null(mode='w'):
> +    """
> +    A portable null file object suitable for use with the subprocess module
> +    """
> +    if hasattr(subprocess, 'DEVNULL'):
> +        return subprocess.DEVNULL
> +    else:
> +        return open(os.path.devnull, mode)
> +
> +
> +def qmp_execute(binary_path, qmp_command):
> +    """
> +    Executes a QMP command on a given QEMU binary
> +
> +    Useful for one-off execution of QEMU binaries to get runtime
> +    information.
> +
> +    @param binary_path: path to a QEMU binary
> +    @param qmp_command: the QMP command
> +    @note: passing arguments to the QMP command is not supported at
> +           this time.
> +    """
> +    try:
> +        tempdir = tempfile.mkdtemp()
> +        monitor_socket = os.path.join(tempdir, 'monitor.sock')
> +        args = [binary_path, '-nodefaults', '-machine', 'none',
> +                '-nographic', '-S', '-qmp', 'unix:%s' % monitor_socket]
> +        monitor = qmp.qmp.QEMUMonitorProtocol(monitor_socket, True)
> +        try:
> +            qemu_proc = subprocess.Popen(args,
> +                                         stdin=subprocess.PIPE,
> +                                         stdout=subprocess.PIPE,
> +                                         stderr=subprocess_dev_null(),
> +                                         universal_newlines=True)
> +        except OSError:
> +            return None
> +        monitor.accept()
> +        res = monitor.cmd(qmp_command)
> +        monitor.cmd("quit")
> +        qemu_proc.wait()
> +        monitor.close()
> +        return res.get("return", None)
> +    finally:
> +        shutil.rmtree(tempdir)
> +
> +
> +def qemu_bin_probe_arch(binary_path):
> +    """
> +    Probes the architecture from the QEMU binary
> +
> +    @returns: either the probed arch or None
> +    @rtype: str or None
> +    @raises: QEMUMachineProbeError
> +    """
> +    res = qmp_execute(binary_path, "query-target")
> +    if res is None:
> +        raise QEMUMachineProbeError('Failed to probe the QEMU arch by querying'
> +                                    ' the target of binary "%s"' % binary_path)
> +    return res.get("arch", None)
> +
> +
> +class QEMU(unittest.TestCase):

'QEMU' is too generic, what is the intended tests to do here? It seems to be
more about exercising the set_console() method.

Any test should be added to tests/, not scripts/.

> +
> +
> +    TEST_ARCH_MACHINE_CONSOLES = {
> +        'alpha': ['clipper'],
> +        'mips': ['malta'],
> +        'x86_64': ['isapc',
> +                   'pc', 'pc-0.10', 'pc-0.11', 'pc-0.12', 'pc-0.13',
> +                   'pc-0.14', 'pc-0.15', 'pc-1.0', 'pc-1.1', 'pc-1.2',
> +                   'pc-1.3',
> +                   'pc-i440fx-1.4', 'pc-i440fx-1.5', 'pc-i440fx-1.6',
> +                   'pc-i440fx-1.7', 'pc-i440fx-2.0', 'pc-i440fx-2.1',
> +                   'pc-i440fx-2.10', 'pc-i440fx-2.11', 'pc-i440fx-2.2',
> +                   'pc-i440fx-2.3', 'pc-i440fx-2.4', 'pc-i440fx-2.5',
> +                   'pc-i440fx-2.6', 'pc-i440fx-2.7', 'pc-i440fx-2.8',
> +                   'pc-i440fx-2.9', 'pc-q35-2.10', 'pc-q35-2.11',
> +                   'q35', 'pc-q35-2.4', 'pc-q35-2.5', 'pc-q35-2.6',
> +                   'pc-q35-2.7', 'pc-q35-2.8', 'pc-q35-2.9'],
> +        'ppc64': ['40p', 'powernv', 'prep', 'pseries', 'pseries-2.1',
> +                  'pseries-2.2', 'pseries-2.3', 'pseries-2.4', 'pseries-2.5',
> +                  'pseries-2.6', 'pseries-2.7', 'pseries-2.8', 'pseries-2.9',
> +                  'pseries-2.10', 'pseries-2.11', 'pseries-2.12'],
> +        's390x': ['s390-ccw-virtio', 's390-ccw-virtio-2.4',
> +                  's390-ccw-virtio-2.5', 's390-ccw-virtio-2.6',
> +                  's390-ccw-virtio-2.7', 's390-ccw-virtio-2.8',
> +                  's390-ccw-virtio-2.9', 's390-ccw-virtio-2.10',
> +                  's390-ccw-virtio-2.11', 's390-ccw-virtio-2.12']
> +    }
> +
> +
> +    def test_set_console(self):
> +        for machines in QEMU.TEST_ARCH_MACHINE_CONSOLES.values():
> +            for machine in machines:
> +                qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +                qemu_machine.set_machine(machine)
> +                qemu_machine.set_console()
> +
> +    def test_set_console_no_machine(self):
> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
> +                          qemu_machine.set_console)
> +
> +    def test_set_console_no_machine_match(self):
> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
> +        qemu_machine.set_machine('unknown-machine-model')
> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
> +                          qemu_machine.set_console)
> +
> +    @unittest.skipUnless(get_built_qemu_binaries(),
> +                         "Could not find any QEMU binaries built to use on "
> +                         "console check")
> +    def test_set_console_launch(self):
> +        for binary in get_built_qemu_binaries():
> +            probed_arch = qemu_bin_probe_arch(binary)
> +            for machine in QEMU.TEST_ARCH_MACHINE_CONSOLES.get(probed_arch, []):
> +                qemu_machine = qemu.QEMUMachine(binary)
> +
> +                # the following workarounds are target specific required for
> +                # this test.  users are of QEMUMachine are expected to deal with
> +                # target specific requirements just the same in their own code
> +                cap_htm_off = ('pseries-2.7', 'pseries-2.8', 'pseries-2.9',
> +                               'pseries-2.10', 'pseries-2.11', 'pseries-2.12')
> +                if probed_arch == 'ppc64' and machine in cap_htm_off:
> +                    qemu_machine._machine = machine   # pylint: disable=W0212
> +                    qemu_machine.args.extend(['-machine',
> +                                              '%s,cap-htm=off' % machine])
> +                elif probed_arch == 's390x':
> +                    qemu_machine.set_machine(machine)
> +                    qemu_machine.args.append('-nodefaults')
> +                elif probed_arch == 'mips':
> +                    qemu_machine.set_machine(machine)
> +                    qemu_machine.args.extend(['-bios', os.path.devnull])
> +                else:
> +                    qemu_machine.set_machine(machine)
> +
> +                qemu_machine.set_console()
> +                qemu_machine.launch()
> +                qemu_machine.shutdown()
> -- 
> 2.17.0
> 

Fam

Re: [Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> TL;DR
> =====
> 
> Another version, with a minimalist approach, to the acceptance tests
> infrastructure for QEMU, based on the Avocado Testing Framework.
> 
> Background
> ==========
> 
> The previous version, still considered an RFC, was sent to the list by
> Eduardo[1] was based on the work held in Amador's branch[2].  After
> reviewing in under a different light, including the experiences
> done and reported by Philippe[3].
> 
> Differences from previous versions
> ==================================
> 
> The main difference is that this series include only the minimal
> changes deemed necessary to have a starting point.  I like to think
> that it's better connected to the QEMU community and project needs,
> and will hopefully allow for a more organic growth.
> 
> Since this version has less features than the previous versions,
> provided it's accepted, these are the next probable development tasks:
> 
>  * Provide a simple variants mechanism to allow the same tests to be
>    run under different targets, machine models and devices (present on
>    the previous versions as a "YAML to Mux" file with architecture
>    definitions)
>  * Implement QEMUMachine migration support (present on the previous
>    version in the "avocado_qemu.test._VM" class)
>  * Implement Guest OS image selection and download (mostly an Avocado
>    feature, paired with a parameter convention and cloud-init support
>    code)
>  * Implement interactive support for Guest OS sessions (present on
>    the previous versions, supported by the aexpect Python module)
> 
> Even though this version shares very little (if any) code with the
> previous versions, the following is a list of noteworthy changes:
> 
>  * Tests directory is now "tests/acceptance" (was "tests/avocado")
>  * Base test class is now "avocado_qemu.Test" (was
>    "avocado_qemu.test.QemuTest")
>  * Base test class is now hosted in "avocado_qemu/__init__.py" (was
>    "avocado_qemu/test.py")
>  * Direct use of "qemu.QEMUMachine", that is, the
>    avocado_qemu.test._VM class is gone
>  * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
>    QEMU binary on a custom system location it's necessary to use the
>    "qemu_bin" parameter
>  * Example test in README.rst is distributed as a real test
>    ("test_version.py")
>  * A new "Linux boot console" test, loosely modeled after Phillipe's
>    use case

I like the direction this series is leading us to.  Making it easy to write
'correct' and 'effective' tests is the most important thing IMO, and the few
tests you add here look promising.

I'm also curious to see a test case involving a guest OS image. (I'm also open to
a re-implementation of tests/vm/basevm.py with Avocado, if you find it
worthwhile.)

Fam

> 
> Commit summary
> ==============
> 
> Cleber Rosa (5):
>   Add functional/acceptance tests infrastructure
>   scripts/qemu.py: allow adding to the list of extra arguments
>   Acceptance tests: add quick VNC tests
>   scripts/qemu.py: introduce set_console() method
>   Acceptance tests: add Linux kernel boot and console checking test
> 
>  scripts/qemu.py                             | 103 +++++++++++++++-
>  scripts/test_qemu.py                        | 176 +++++++++++++++++++++++++++
>  tests/acceptance/README.rst                 | 141 +++++++++++++++++++++
>  tests/acceptance/avocado_qemu/__init__.py   |  45 +++++++
>  tests/acceptance/test_boot_linux_console.py |  37 ++++++
>  tests/acceptance/test_version.py            |  13 ++
>  tests/acceptance/test_vnc.py                |  50 ++++++++
>  7 files changed, 564 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
>  create mode 100644 tests/acceptance/README.rst
>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>  create mode 100644 tests/acceptance/test_boot_linux_console.py
>  create mode 100644 tests/acceptance/test_version.py
>  create mode 100644 tests/acceptance/test_vnc.py
> 
> ---
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
> [2] https://github.com/apahim/qemu/commits/avocado_qemu
> [3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html
> 

Re: [Qemu-devel] [PATCH 1/5] Add functional/acceptance tests infrastructure

[Cleber Rosa]

On 05/25/2018 01:33 AM, Fam Zheng wrote:
> On Thu, 05/24 20:58, Cleber Rosa wrote:
>> 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>
>> ---
>>  tests/acceptance/README.rst               | 141 ++++++++++++++++++++++
> 
> Could you add the actual doc text to docs/devel/testing.rst and reference it
> here?
> 

Yes, sure.

>>  tests/acceptance/avocado_qemu/__init__.py |  45 +++++++
>>  tests/acceptance/test_version.py          |  13 ++
>>  3 files changed, 199 insertions(+)
>>  create mode 100644 tests/acceptance/README.rst
>>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>>  create mode 100644 tests/acceptance/test_version.py
>>
>> diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
>> new file mode 100644
>> index 0000000000..f1434177da
>> --- /dev/null
>> +++ b/tests/acceptance/README.rst
>> @@ -0,0 +1,141 @@
>> +==============================================
>> + Acceptance tests using the Avocado Framework
>> +==============================================
>> +
>> +This 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.
>> +
>> +The tests are written using the Avocado Testing Framework, in
>> +conjunction with a the ``avocado_qemu.Test`` class, distributed here.
>> +
>> +Installation
>> +============
>> +
>> +To install Avocado and its dependencies, run::
>> +
>> +  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::
>> +
>> +  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::
>> +
>> +  avocado run test_version.py
>> +
>> +To run all tests in the current directory, tagged in a particular way,
>> +run::
>> +
>> +  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::
>> +
>> +  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/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>> new file mode 100644
>> index 0000000000..dcd58047a3
>> --- /dev/null
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -0,0 +1,45 @@
> 
> A copy right header is necessary here.
> 

OK.

>> +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 = 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/test_version.py b/tests/acceptance/test_version.py
>> new file mode 100644
>> index 0000000000..e397f07d83
>> --- /dev/null
>> +++ b/tests/acceptance/test_version.py
>> @@ -0,0 +1,13 @@
>> +from avocado_qemu import Test
> 
> Same here.
> 

Sure.

- Cleber.

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

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

[Cleber Rosa]

On 05/25/2018 01:37 AM, Fam Zheng wrote:
> On Thu, 05/24 20:58, Cleber Rosa wrote:
>> This patch adds a few simple behavior tests for VNC.  These tests
>> introduce manipulation of the QEMUMachine arguments, by setting
>> the arguments, instead of adding to the existing ones.
> 
> I'm confused by this. The code uses 'add_args', so it does add to the arguments,
> no?
> 

And you should be.  I changed the code to just add args, and forgot to
update the commit message.  If a better example comes up that requires
setting arguments, I'll get back to this.

>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>  create mode 100644 tests/acceptance/test_vnc.py
>>
>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
>> new file mode 100644
>> index 0000000000..9d9a35cf55
>> --- /dev/null
>> +++ b/tests/acceptance/test_vnc.py
>> @@ -0,0 +1,50 @@
> 
> Copyright header is missing here too.
> 

Indeed.

> Fam
> 
>> +from avocado_qemu import Test
>> +
>> +
>> +class Vnc(Test):
> 
> Should VncTest be a better class name?
> 

I'm favoring simpler names.  If you think about the complete test names,
it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc".

That's actually an interesting point: how would you feel about dropping
the "test_" prefix from the file names?

- Cleber.

>> +    """
>> +    :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
>>
> 
> Fam
> 

Re: [Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Cleber Rosa]

On 05/25/2018 01:47 AM, Fam Zheng wrote:
> On Thu, 05/24 20:58, Cleber Rosa wrote:
>> The set_console() method is intended to ease higher level use cases
>> that require a console device.
>>
>> The amount of inteligence is limited on purpose, requiring either the
>> device type explicitly, or the existence of a machine (pattern)
>> definition.
>>

Typo here (s/inteligence/intelligence").

>> 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 +++++++++++++++++++++++-
>>  scripts/test_qemu.py | 176 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 272 insertions(+), 1 deletion(-)
>>  create mode 100644 scripts/test_qemu.py
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 7813dd45ad..89db5bc6b2 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
>>          '''
>>          return 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
>> diff --git a/scripts/test_qemu.py b/scripts/test_qemu.py
>> new file mode 100644
>> index 0000000000..5e016d3751
>> --- /dev/null
>> +++ b/scripts/test_qemu.py
>> @@ -0,0 +1,176 @@
> 
> Again, a file header is missing.
> 

Yep, adding one.

>> +import sys
>> +import os
>> +import glob
>> +import unittest
>> +import shutil
>> +import tempfile
>> +import subprocess
>> +
>> +import qemu
>> +import qmp.qmp
>> +
>> +
>> +class QEMUMachineProbeError(Exception):
>> +    """
>> +    Exception raised when a probe a fails to be deterministic
>> +    """
>> +
>> +
>> +def get_built_qemu_binaries(root_dir=None):
>> +    """
>> +    Attempts to find QEMU binaries in a tree
>> +
>> +    If root_dir is None, it will attempt to find the binaries at the
>> +    source tree.  It's possible to override it by setting the environment
>> +    variable QEMU_ROOT_DIR.
>> +    """
>> +    if root_dir is None:
>> +        src_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
>> +        root_dir = os.environ.get("QEMU_ROOT_DIR", src_dir)
>> +    binaries = glob.glob(os.path.join(root_dir, '*-softmmu/qemu-system-*'))
>> +    if 'win' in sys.platform:
>> +        bin_filter = lambda x: x.endswith(".exe")
>> +    else:
>> +        bin_filter = lambda x: not x.endswith(".exe")
>> +    return [_ for _ in binaries if bin_filter(_)]
>> +
>> +
>> +def subprocess_dev_null(mode='w'):
>> +    """
>> +    A portable null file object suitable for use with the subprocess module
>> +    """
>> +    if hasattr(subprocess, 'DEVNULL'):
>> +        return subprocess.DEVNULL
>> +    else:
>> +        return open(os.path.devnull, mode)
>> +
>> +
>> +def qmp_execute(binary_path, qmp_command):
>> +    """
>> +    Executes a QMP command on a given QEMU binary
>> +
>> +    Useful for one-off execution of QEMU binaries to get runtime
>> +    information.
>> +
>> +    @param binary_path: path to a QEMU binary
>> +    @param qmp_command: the QMP command
>> +    @note: passing arguments to the QMP command is not supported at
>> +           this time.
>> +    """
>> +    try:
>> +        tempdir = tempfile.mkdtemp()
>> +        monitor_socket = os.path.join(tempdir, 'monitor.sock')
>> +        args = [binary_path, '-nodefaults', '-machine', 'none',
>> +                '-nographic', '-S', '-qmp', 'unix:%s' % monitor_socket]
>> +        monitor = qmp.qmp.QEMUMonitorProtocol(monitor_socket, True)
>> +        try:
>> +            qemu_proc = subprocess.Popen(args,
>> +                                         stdin=subprocess.PIPE,
>> +                                         stdout=subprocess.PIPE,
>> +                                         stderr=subprocess_dev_null(),
>> +                                         universal_newlines=True)
>> +        except OSError:
>> +            return None
>> +        monitor.accept()
>> +        res = monitor.cmd(qmp_command)
>> +        monitor.cmd("quit")
>> +        qemu_proc.wait()
>> +        monitor.close()
>> +        return res.get("return", None)
>> +    finally:
>> +        shutil.rmtree(tempdir)
>> +
>> +
>> +def qemu_bin_probe_arch(binary_path):
>> +    """
>> +    Probes the architecture from the QEMU binary
>> +
>> +    @returns: either the probed arch or None
>> +    @rtype: str or None
>> +    @raises: QEMUMachineProbeError
>> +    """
>> +    res = qmp_execute(binary_path, "query-target")
>> +    if res is None:
>> +        raise QEMUMachineProbeError('Failed to probe the QEMU arch by querying'
>> +                                    ' the target of binary "%s"' % binary_path)
>> +    return res.get("arch", None)
>> +
>> +
>> +class QEMU(unittest.TestCase):
> 
> 'QEMU' is too generic, what is the intended tests to do here? It seems to be
> more about exercising the set_console() method.
> 

Yes, but again, I'm favoring simpler names when the scope is supposed to
be limited.  The chances of a name clash here are close to none IMO.  I
don't think this class would be reused elsewhere, or a class with the
same name would be imported here.

> Any test should be added to tests/, not scripts/.
> 

One of the reasons for this file to be in this patch was to generate
this exact discussion.  "qemu.py" itself is not a script, but a
module/library.  Still it lacks the structure to have accompanying tests.

I was hoping to get away with these tests here, so that they wouldn't be
thrown away, until "qemu.py", "qmp/*.py" and others are given the status
and structure of Python modules.

I can definitely move these to tests/, but how about its relation to the
other tests living there and its activation?  Should we recognize its
existence and add a check-* target?

- Cleber.

>> +
>> +
>> +    TEST_ARCH_MACHINE_CONSOLES = {
>> +        'alpha': ['clipper'],
>> +        'mips': ['malta'],
>> +        'x86_64': ['isapc',
>> +                   'pc', 'pc-0.10', 'pc-0.11', 'pc-0.12', 'pc-0.13',
>> +                   'pc-0.14', 'pc-0.15', 'pc-1.0', 'pc-1.1', 'pc-1.2',
>> +                   'pc-1.3',
>> +                   'pc-i440fx-1.4', 'pc-i440fx-1.5', 'pc-i440fx-1.6',
>> +                   'pc-i440fx-1.7', 'pc-i440fx-2.0', 'pc-i440fx-2.1',
>> +                   'pc-i440fx-2.10', 'pc-i440fx-2.11', 'pc-i440fx-2.2',
>> +                   'pc-i440fx-2.3', 'pc-i440fx-2.4', 'pc-i440fx-2.5',
>> +                   'pc-i440fx-2.6', 'pc-i440fx-2.7', 'pc-i440fx-2.8',
>> +                   'pc-i440fx-2.9', 'pc-q35-2.10', 'pc-q35-2.11',
>> +                   'q35', 'pc-q35-2.4', 'pc-q35-2.5', 'pc-q35-2.6',
>> +                   'pc-q35-2.7', 'pc-q35-2.8', 'pc-q35-2.9'],
>> +        'ppc64': ['40p', 'powernv', 'prep', 'pseries', 'pseries-2.1',
>> +                  'pseries-2.2', 'pseries-2.3', 'pseries-2.4', 'pseries-2.5',
>> +                  'pseries-2.6', 'pseries-2.7', 'pseries-2.8', 'pseries-2.9',
>> +                  'pseries-2.10', 'pseries-2.11', 'pseries-2.12'],
>> +        's390x': ['s390-ccw-virtio', 's390-ccw-virtio-2.4',
>> +                  's390-ccw-virtio-2.5', 's390-ccw-virtio-2.6',
>> +                  's390-ccw-virtio-2.7', 's390-ccw-virtio-2.8',
>> +                  's390-ccw-virtio-2.9', 's390-ccw-virtio-2.10',
>> +                  's390-ccw-virtio-2.11', 's390-ccw-virtio-2.12']
>> +    }
>> +
>> +
>> +    def test_set_console(self):
>> +        for machines in QEMU.TEST_ARCH_MACHINE_CONSOLES.values():
>> +            for machine in machines:
>> +                qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +                qemu_machine.set_machine(machine)
>> +                qemu_machine.set_console()
>> +
>> +    def test_set_console_no_machine(self):
>> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
>> +                          qemu_machine.set_console)
>> +
>> +    def test_set_console_no_machine_match(self):
>> +        qemu_machine = qemu.QEMUMachine('/fake/path/to/binary')
>> +        qemu_machine.set_machine('unknown-machine-model')
>> +        self.assertRaises(qemu.QEMUMachineAddDeviceError,
>> +                          qemu_machine.set_console)
>> +
>> +    @unittest.skipUnless(get_built_qemu_binaries(),
>> +                         "Could not find any QEMU binaries built to use on "
>> +                         "console check")
>> +    def test_set_console_launch(self):
>> +        for binary in get_built_qemu_binaries():
>> +            probed_arch = qemu_bin_probe_arch(binary)
>> +            for machine in QEMU.TEST_ARCH_MACHINE_CONSOLES.get(probed_arch, []):
>> +                qemu_machine = qemu.QEMUMachine(binary)
>> +
>> +                # the following workarounds are target specific required for
>> +                # this test.  users are of QEMUMachine are expected to deal with
>> +                # target specific requirements just the same in their own code
>> +                cap_htm_off = ('pseries-2.7', 'pseries-2.8', 'pseries-2.9',
>> +                               'pseries-2.10', 'pseries-2.11', 'pseries-2.12')
>> +                if probed_arch == 'ppc64' and machine in cap_htm_off:
>> +                    qemu_machine._machine = machine   # pylint: disable=W0212
>> +                    qemu_machine.args.extend(['-machine',
>> +                                              '%s,cap-htm=off' % machine])
>> +                elif probed_arch == 's390x':
>> +                    qemu_machine.set_machine(machine)
>> +                    qemu_machine.args.append('-nodefaults')
>> +                elif probed_arch == 'mips':
>> +                    qemu_machine.set_machine(machine)
>> +                    qemu_machine.args.extend(['-bios', os.path.devnull])
>> +                else:
>> +                    qemu_machine.set_machine(machine)
>> +
>> +                qemu_machine.set_console()
>> +                qemu_machine.launch()
>> +                qemu_machine.shutdown()
>> -- 
>> 2.17.0
>>
> 
> Fam
> 

Re: [Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Cleber Rosa]

On 05/25/2018 02:08 AM, Fam Zheng wrote:
> On Thu, 05/24 20:58, Cleber Rosa wrote:
>> TL;DR
>> =====
>>
>> Another version, with a minimalist approach, to the acceptance tests
>> infrastructure for QEMU, based on the Avocado Testing Framework.
>>
>> Background
>> ==========
>>
>> The previous version, still considered an RFC, was sent to the list by
>> Eduardo[1] was based on the work held in Amador's branch[2].  After
>> reviewing in under a different light, including the experiences
>> done and reported by Philippe[3].
>>
>> Differences from previous versions
>> ==================================
>>
>> The main difference is that this series include only the minimal
>> changes deemed necessary to have a starting point.  I like to think
>> that it's better connected to the QEMU community and project needs,
>> and will hopefully allow for a more organic growth.
>>
>> Since this version has less features than the previous versions,
>> provided it's accepted, these are the next probable development tasks:
>>
>>  * Provide a simple variants mechanism to allow the same tests to be
>>    run under different targets, machine models and devices (present on
>>    the previous versions as a "YAML to Mux" file with architecture
>>    definitions)
>>  * Implement QEMUMachine migration support (present on the previous
>>    version in the "avocado_qemu.test._VM" class)
>>  * Implement Guest OS image selection and download (mostly an Avocado
>>    feature, paired with a parameter convention and cloud-init support
>>    code)
>>  * Implement interactive support for Guest OS sessions (present on
>>    the previous versions, supported by the aexpect Python module)
>>
>> Even though this version shares very little (if any) code with the
>> previous versions, the following is a list of noteworthy changes:
>>
>>  * Tests directory is now "tests/acceptance" (was "tests/avocado")
>>  * Base test class is now "avocado_qemu.Test" (was
>>    "avocado_qemu.test.QemuTest")
>>  * Base test class is now hosted in "avocado_qemu/__init__.py" (was
>>    "avocado_qemu/test.py")
>>  * Direct use of "qemu.QEMUMachine", that is, the
>>    avocado_qemu.test._VM class is gone
>>  * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
>>    QEMU binary on a custom system location it's necessary to use the
>>    "qemu_bin" parameter
>>  * Example test in README.rst is distributed as a real test
>>    ("test_version.py")
>>  * A new "Linux boot console" test, loosely modeled after Phillipe's
>>    use case
> 
> I like the direction this series is leading us to.  Making it easy to write
> 'correct' and 'effective' tests is the most important thing IMO, and the few
> tests you add here look promising.
> 

The soft targets that these adjectives ("easy", "correct" and
"effective") bring, are kind of hard to get right, but I'm really glad
you like what you see.

> I'm also curious to see a test case involving a guest OS image. (I'm also open to
> a re-implementation of tests/vm/basevm.py with Avocado, if you find it
> worthwhile.)
> 

We had such tests in the previous incarnations of this work[1].  But by
doing it in smaller steps, I believe we can achieve a better result,
specially when it comes to the exposed API.

- Cleber.

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03466.html

> Fam
> 
>>
>> Commit summary
>> ==============
>>
>> Cleber Rosa (5):
>>   Add functional/acceptance tests infrastructure
>>   scripts/qemu.py: allow adding to the list of extra arguments
>>   Acceptance tests: add quick VNC tests
>>   scripts/qemu.py: introduce set_console() method
>>   Acceptance tests: add Linux kernel boot and console checking test
>>
>>  scripts/qemu.py                             | 103 +++++++++++++++-
>>  scripts/test_qemu.py                        | 176 +++++++++++++++++++++++++++
>>  tests/acceptance/README.rst                 | 141 +++++++++++++++++++++
>>  tests/acceptance/avocado_qemu/__init__.py   |  45 +++++++
>>  tests/acceptance/test_boot_linux_console.py |  37 ++++++
>>  tests/acceptance/test_version.py            |  13 ++
>>  tests/acceptance/test_vnc.py                |  50 ++++++++
>>  7 files changed, 564 insertions(+), 1 deletion(-)
>>  create mode 100644 scripts/test_qemu.py
>>  create mode 100644 tests/acceptance/README.rst
>>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>>  create mode 100644 tests/acceptance/test_boot_linux_console.py
>>  create mode 100644 tests/acceptance/test_version.py
>>  create mode 100644 tests/acceptance/test_vnc.py
>>
>> ---
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
>> [2] https://github.com/apahim/qemu/commits/avocado_qemu
>> [3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html
>>
> 

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

[Eduardo Habkost]

On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote:
> 
> 
> On 05/25/2018 01:37 AM, Fam Zheng wrote:
> > On Thu, 05/24 20:58, Cleber Rosa wrote:
> >> This patch adds a few simple behavior tests for VNC.  These tests
> >> introduce manipulation of the QEMUMachine arguments, by setting
> >> the arguments, instead of adding to the existing ones.
> > 
> > I'm confused by this. The code uses 'add_args', so it does add to the arguments,
> > no?
> > 
> 
> And you should be.  I changed the code to just add args, and forgot to
> update the commit message.  If a better example comes up that requires
> setting arguments, I'll get back to this.
> 
> >>
> >> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> >> ---
> >>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >>  create mode 100644 tests/acceptance/test_vnc.py
> >>
> >> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
> >> new file mode 100644
> >> index 0000000000..9d9a35cf55
> >> --- /dev/null
> >> +++ b/tests/acceptance/test_vnc.py
> >> @@ -0,0 +1,50 @@
> > 
> > Copyright header is missing here too.
> > 
> 
> Indeed.
> 
> > Fam
> > 
> >> +from avocado_qemu import Test
> >> +
> >> +
> >> +class Vnc(Test):
> > 
> > Should VncTest be a better class name?
> > 
> 
> I'm favoring simpler names.  If you think about the complete test names,
> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc".
> 
> That's actually an interesting point: how would you feel about dropping
> the "test_" prefix from the file names?

I would like this.  The file is already inside a tests/acceptance
directory.

About the class name, I would be less surprised when reading the
code if it was called "VncTest", but I won't object to "Vnc" if
you prefer it.

-- 
Eduardo

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

[Cleber Rosa]

On 05/29/2018 10:32 AM, Eduardo Habkost wrote:
> On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote:
>>
>>
>> On 05/25/2018 01:37 AM, Fam Zheng wrote:
>>> On Thu, 05/24 20:58, Cleber Rosa wrote:
>>>> This patch adds a few simple behavior tests for VNC.  These tests
>>>> introduce manipulation of the QEMUMachine arguments, by setting
>>>> the arguments, instead of adding to the existing ones.
>>>
>>> I'm confused by this. The code uses 'add_args', so it does add to the arguments,
>>> no?
>>>
>>
>> And you should be.  I changed the code to just add args, and forgot to
>> update the commit message.  If a better example comes up that requires
>> setting arguments, I'll get back to this.
>>
>>>>
>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>> ---
>>>>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 50 insertions(+)
>>>>  create mode 100644 tests/acceptance/test_vnc.py
>>>>
>>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
>>>> new file mode 100644
>>>> index 0000000000..9d9a35cf55
>>>> --- /dev/null
>>>> +++ b/tests/acceptance/test_vnc.py
>>>> @@ -0,0 +1,50 @@
>>>
>>> Copyright header is missing here too.
>>>
>>
>> Indeed.
>>
>>> Fam
>>>
>>>> +from avocado_qemu import Test
>>>> +
>>>> +
>>>> +class Vnc(Test):
>>>
>>> Should VncTest be a better class name?
>>>
>>
>> I'm favoring simpler names.  If you think about the complete test names,
>> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc".
>>
>> That's actually an interesting point: how would you feel about dropping
>> the "test_" prefix from the file names?
> 
> I would like this.  The file is already inside a tests/acceptance
> directory.
> 
> About the class name, I would be less surprised when reading the
> code if it was called "VncTest", but I won't object to "Vnc" if
> you prefer it.
> 

We already gained one simplification by dropping the "test_" file
prefix, so I wouldn't mind adding the "Test" suffix to the test classes.

Now, before I do, consider that when reading the code you'll find:

   class Vnc(Test):
      ...

Which IMO makes it pretty clear that this is a test class.  And as you
said it yourself, those files are already in a tests/acceptance directory.

So back to you (for the last time, I promise): would you like me to add
the Test suffix to all test classes?

- Cleber.

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

[Eduardo Habkost]

On Tue, May 29, 2018 at 10:59:37AM -0400, Cleber Rosa wrote:
> 
> 
> On 05/29/2018 10:32 AM, Eduardo Habkost wrote:
> > On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote:
> >>
> >>
> >> On 05/25/2018 01:37 AM, Fam Zheng wrote:
> >>> On Thu, 05/24 20:58, Cleber Rosa wrote:
> >>>> This patch adds a few simple behavior tests for VNC.  These tests
> >>>> introduce manipulation of the QEMUMachine arguments, by setting
> >>>> the arguments, instead of adding to the existing ones.
> >>>
> >>> I'm confused by this. The code uses 'add_args', so it does add to the arguments,
> >>> no?
> >>>
> >>
> >> And you should be.  I changed the code to just add args, and forgot to
> >> update the commit message.  If a better example comes up that requires
> >> setting arguments, I'll get back to this.
> >>
> >>>>
> >>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> >>>> ---
> >>>>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 50 insertions(+)
> >>>>  create mode 100644 tests/acceptance/test_vnc.py
> >>>>
> >>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
> >>>> new file mode 100644
> >>>> index 0000000000..9d9a35cf55
> >>>> --- /dev/null
> >>>> +++ b/tests/acceptance/test_vnc.py
> >>>> @@ -0,0 +1,50 @@
> >>>
> >>> Copyright header is missing here too.
> >>>
> >>
> >> Indeed.
> >>
> >>> Fam
> >>>
> >>>> +from avocado_qemu import Test
> >>>> +
> >>>> +
> >>>> +class Vnc(Test):
> >>>
> >>> Should VncTest be a better class name?
> >>>
> >>
> >> I'm favoring simpler names.  If you think about the complete test names,
> >> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc".
> >>
> >> That's actually an interesting point: how would you feel about dropping
> >> the "test_" prefix from the file names?
> > 
> > I would like this.  The file is already inside a tests/acceptance
> > directory.
> > 
> > About the class name, I would be less surprised when reading the
> > code if it was called "VncTest", but I won't object to "Vnc" if
> > you prefer it.
> > 
> 
> We already gained one simplification by dropping the "test_" file
> prefix, so I wouldn't mind adding the "Test" suffix to the test classes.
> 
> Now, before I do, consider that when reading the code you'll find:
> 
>    class Vnc(Test):
>       ...
> 
> Which IMO makes it pretty clear that this is a test class.  And as you
> said it yourself, those files are already in a tests/acceptance directory.

Yes, he context makes it clear it's a test class, but the pattern
is still unusual because class names are normally meaningful when
they are referenced elsewhere.  However, in this case it doesn't
matter too much because there are no references to "Vnc" in the
rest of the code.


> 
> So back to you (for the last time, I promise): would you like me to add
> the Test suffix to all test classes?

It's up to you.  I'd add them, but I won't complain too loudly if
you prefer to not add it.  :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Eduardo Habkost]

On Fri, May 25, 2018 at 12:57:54PM -0400, Cleber Rosa wrote:
> On 05/25/2018 01:47 AM, Fam Zheng wrote:
[...]
> >> +class QEMU(unittest.TestCase):
> > 
> > 'QEMU' is too generic, what is the intended tests to do here? It seems to be
> > more about exercising the set_console() method.
> > 
> 
> Yes, but again, I'm favoring simpler names when the scope is supposed to
> be limited.  The chances of a name clash here are close to none IMO.  I
> don't think this class would be reused elsewhere, or a class with the
> same name would be imported here.
> 
> > Any test should be added to tests/, not scripts/.
> > 
> 
> One of the reasons for this file to be in this patch was to generate
> this exact discussion.  "qemu.py" itself is not a script, but a
> module/library.  Still it lacks the structure to have accompanying tests.
> 
> I was hoping to get away with these tests here, so that they wouldn't be
> thrown away, until "qemu.py", "qmp/*.py" and others are given the status
> and structure of Python modules.
> 
> I can definitely move these to tests/, but how about its relation to the
> other tests living there and its activation?  Should we recognize its
> existence and add a check-* target?

"scripts/test_qemu.py" makes it look like a script that will test
QEMU.  While we don't rearrange the Python modules to follow a
more Pythonic structure, probably moving it to tests/ is the best
option we have.  Maybe tests/python?

Wherever we store the test code, running those tests on
"make check" is a good idea.

But I wouldn't like this to delay the inclusion of this series.
If rearranging the Python modules would make the job easier,
including the test code only later would be a reasonable option,
too.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Cleber Rosa]

On 05/29/2018 03:06 PM, Eduardo Habkost wrote:
> On Fri, May 25, 2018 at 12:57:54PM -0400, Cleber Rosa wrote:
>> On 05/25/2018 01:47 AM, Fam Zheng wrote:
> [...]
>>>> +class QEMU(unittest.TestCase):
>>>
>>> 'QEMU' is too generic, what is the intended tests to do here? It seems to be
>>> more about exercising the set_console() method.
>>>
>>
>> Yes, but again, I'm favoring simpler names when the scope is supposed to
>> be limited.  The chances of a name clash here are close to none IMO.  I
>> don't think this class would be reused elsewhere, or a class with the
>> same name would be imported here.
>>
>>> Any test should be added to tests/, not scripts/.
>>>
>>
>> One of the reasons for this file to be in this patch was to generate
>> this exact discussion.  "qemu.py" itself is not a script, but a
>> module/library.  Still it lacks the structure to have accompanying tests.
>>
>> I was hoping to get away with these tests here, so that they wouldn't be
>> thrown away, until "qemu.py", "qmp/*.py" and others are given the status
>> and structure of Python modules.
>>
>> I can definitely move these to tests/, but how about its relation to the
>> other tests living there and its activation?  Should we recognize its
>> existence and add a check-* target?
> 
> "scripts/test_qemu.py" makes it look like a script that will test
> QEMU.  While we don't rearrange the Python modules to follow a
> more Pythonic structure, probably moving it to tests/ is the best
> option we have.  Maybe tests/python?
> 
> Wherever we store the test code, running those tests on
> "make check" is a good idea.
> 
> But I wouldn't like this to delay the inclusion of this series.
> If rearranging the Python modules would make the job easier,
> including the test code only later would be a reasonable option,
> too.
> 

We already have a "probable next steps" planning (see PATCH 0), and we
can include a "Python module reorganization" to it.  So, I'm dropping
this test on v3 and we'll add it later, at a better location.

- Cleber.

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

[Cleber Rosa]

On 05/29/2018 01:52 PM, Eduardo Habkost wrote:
> On Tue, May 29, 2018 at 10:59:37AM -0400, Cleber Rosa wrote:
>>
>>
>> On 05/29/2018 10:32 AM, Eduardo Habkost wrote:
>>> On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote:
>>>>
>>>>
>>>> On 05/25/2018 01:37 AM, Fam Zheng wrote:
>>>>> On Thu, 05/24 20:58, Cleber Rosa wrote:
>>>>>> This patch adds a few simple behavior tests for VNC.  These tests
>>>>>> introduce manipulation of the QEMUMachine arguments, by setting
>>>>>> the arguments, instead of adding to the existing ones.
>>>>>
>>>>> I'm confused by this. The code uses 'add_args', so it does add to the arguments,
>>>>> no?
>>>>>
>>>>
>>>> And you should be.  I changed the code to just add args, and forgot to
>>>> update the commit message.  If a better example comes up that requires
>>>> setting arguments, I'll get back to this.
>>>>
>>>>>>
>>>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>>>> ---
>>>>>>  tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 50 insertions(+)
>>>>>>  create mode 100644 tests/acceptance/test_vnc.py
>>>>>>
>>>>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
>>>>>> new file mode 100644
>>>>>> index 0000000000..9d9a35cf55
>>>>>> --- /dev/null
>>>>>> +++ b/tests/acceptance/test_vnc.py
>>>>>> @@ -0,0 +1,50 @@
>>>>>
>>>>> Copyright header is missing here too.
>>>>>
>>>>
>>>> Indeed.
>>>>
>>>>> Fam
>>>>>
>>>>>> +from avocado_qemu import Test
>>>>>> +
>>>>>> +
>>>>>> +class Vnc(Test):
>>>>>
>>>>> Should VncTest be a better class name?
>>>>>
>>>>
>>>> I'm favoring simpler names.  If you think about the complete test names,
>>>> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc".
>>>>
>>>> That's actually an interesting point: how would you feel about dropping
>>>> the "test_" prefix from the file names?
>>>
>>> I would like this.  The file is already inside a tests/acceptance
>>> directory.
>>>
>>> About the class name, I would be less surprised when reading the
>>> code if it was called "VncTest", but I won't object to "Vnc" if
>>> you prefer it.
>>>
>>
>> We already gained one simplification by dropping the "test_" file
>> prefix, so I wouldn't mind adding the "Test" suffix to the test classes.
>>
>> Now, before I do, consider that when reading the code you'll find:
>>
>>    class Vnc(Test):
>>       ...
>>
>> Which IMO makes it pretty clear that this is a test class.  And as you
>> said it yourself, those files are already in a tests/acceptance directory.
> 
> Yes, he context makes it clear it's a test class, but the pattern
> is still unusual because class names are normally meaningful when
> they are referenced elsewhere.  However, in this case it doesn't
> matter too much because there are no references to "Vnc" in the
> rest of the code.
> 
> 
>>
>> So back to you (for the last time, I promise): would you like me to add
>> the Test suffix to all test classes?
> 
> It's up to you.  I'd add them, but I won't complain too loudly if
> you prefer to not add it.  :)
> 

OK.  I'll listen to my intuition here, as it's telling me pretty loudly
that it will be seen as a good thing "soon", when we're looking at the
result of hundreds of tests! ;)

- Cleber.