[Qemu-devel] [RFC PATCH 1/3] acceptance tests: Add SeaBIOS boot and debug console checking test

* [Qemu-devel] [RFC PATCH 1/3] acceptance tests: Add SeaBIOS boot and debug console checking test
       [not found] ` <20180928003058.12786-2-philmd@redhat.com>
@ 2018-10-02 23:26   ` Cleber Rosa
  2018-10-03  0:00     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 9+ messages in thread
From: Cleber Rosa @ 2018-10-02 23:26 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Eduardo Habkost, Caio Carrara
  Cc: qemu-devel, Marc-André Lureau, Gerd Hoffmann, Laszlo Ersek

On 9/27/18 8:30 PM, Philippe Mathieu-Daudé wrote:
> This test boots SeaBIOS and check the debug console (I/O port on the ISA bus)
> reports enough information on the initialized devices.
> 
> Example:
> 
>     $ avocado --show=debugcon run tests/acceptance/boot_firmware.py
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> - can we avoid the time.time() 2nd timeout check?
> 

The problem here is that the "draining" of the console is done
synchronously and at the same time the "if 'No bootable device.' in msg"
check is performed.

If we split those, it could become a simple function that can be given
to wait_for():

https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#avocado.utils.wait.wait_for

Something like:

   if not wait_for(check_bootable(), 5):
       self.fail("Reason")

Greatly simplifying the code.

> - can we avoid tempfile.mkdtemp() + shutil.rmtree() with Python2?
>   (using context manager)

Yep, workdir is your friend.  More on that later.

> ---
>  tests/acceptance/boot_firmware.py | 72 +++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 tests/acceptance/boot_firmware.py
> 
> diff --git a/tests/acceptance/boot_firmware.py b/tests/acceptance/boot_firmware.py
> new file mode 100644
> index 0000000000..a41e04936d
> --- /dev/null
> +++ b/tests/acceptance/boot_firmware.py
> @@ -0,0 +1,72 @@
> +# coding=utf-8
> +#
> +# Functional test that boots SeaBIOS and checks the console
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Philippe Mathieu-Daudé <philmd@redhat.com>
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import os
> +import time
> +import shutil
> +import logging
> +import tempfile
> +
> +from avocado_qemu import Test
> +
> +
> +class BootFirmwareX86(Test):
> +    """
> +    Boots a firmware on a default PC machine and checks the debug console is
> +    operational
> +
> +    :avocado: enable
> +    :avocado: tags=x86_64,quick

Eventually, we'll need to have a predictable definition to, at least,
some of the tags.  I mean, I agree this *should* be a quick by test
(considering it's a functional test), but eventually we may want to set
some ranges for what we consider "quick", "slow" or "I'm gonna take my
time, don't wait on me". :)

> +    """
> +
> +    timeout = 15
> +
> +    def test_seabios(self):
> +        tmpdirname = tempfile.mkdtemp()

Every Avocado test has a "workdir" that can be used for temporary
content.  That property points to a directory will be discarded after
the test has finished.  Documentation is here:

https://avocado-framework.readthedocs.io/en/65.0/api/test/avocado.html#avocado.Test.workdir

> +        debugcon_path = os.path.join(tmpdirname, 'debugconsole.log')
> +        serial_logger = logging.getLogger('serial')
> +        debugcon_logger = logging.getLogger('debugcon')
> +
> +        self.vm.set_machine('pc')
> +        self.vm.set_console()
> +        self.vm.add_args('-nographic',
> +                         '-net', 'none',
> +                         '-global', 'isa-debugcon.iobase=0x402',
> +                         '-debugcon', 'file:%s' % debugcon_path)
> +        self.vm.launch()
> +        console = self.vm.console_socket.makefile()
> +
> +        # serial console checks
> +        timeout = time.time() + 5
> +        while True:
> +            msg = console.readline()
> +            if not '\x1b' in msg: # ignore ANSI sequences
> +                serial_logger.debug(msg.strip())
> +            if 'No bootable device.' in msg:
> +                break
> +            if time.time() > timeout:
> +                self.fail('SeaBIOS failed to boot?')

It looks like tests such as this could benefit from having an async
method of draining a given file (descriptor or object) and putting that
into a log.  Then, the boilerplate code of reading from a file and
writing to a log could be avoided.

In theory, Avocado has such a tool:

https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#avocado.utils.process.FDDrainer

But it needs some TLC when it comes to being used on a test to really
remove boilerplate code. For instance, instead of having to do:

    debugcon_path = os.path.join(self.workdir, 'debug_console.log')
    debugcon_logger = logging.getLogger('debugcon')
    ...
    lines = open(debugcon_path).readlines()
    for msg in lines:
        debugcon_logger.debug(msg.strip())

One should be able to do:

   # debug_console.log is created with the content of debugcon_path
   self.log_file(debugcon_path, "debug_console")

> +
> +        # debug console checks
> +        expected = [
> +            'Running on QEMU (i440fx)',
> +            'Turning on vga text mode console',
> +            'Found 1 lpt ports',
> +            'Found 1 serial ports',
> +            'PS2 keyboard initialized',
> +        ]
> +        lines = open(debugcon_path).readlines()
> +        for msg in lines:
> +            debugcon_logger.debug(msg.strip())
> +        for line in expected:
> +            if line + '\n' not in lines:
> +                self.fail('missing: %s' % line)

A slightly more Pythonic version would be:

   with open(debugcon_path) as debugcon:
      content = debugcon.readlines()
      for exp in expected:
          self.assertIn(expected, content)

> +        shutil.rmtree(tmpdirname, ignore_errors=True)
> 

And by using workdir you don't need this manual cleanup.

^ permalink raw reply	[flat|nested] 9+ messages in thread