Re: [Qemu-devel] [RFC PATCH 2/3] acceptance tests: Add EDK2 OVMF boot and debug console checking test

[Cleber Rosa]

On 9/27/18 8:30 PM, Philippe Mathieu-Daudé wrote:
> This test boots OVMF and check the debug console (I/O port on the ISA bus)
> report enough information on the initialized devices.
> 
> Example:
> 
>     $ avocado --show=QMP,serial run tests/acceptance/boot_firmware.py
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> - how to refactor common code from test_seabios() (previous patch)?
> ---
>  tests/acceptance/boot_firmware.py | 52 +++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/tests/acceptance/boot_firmware.py 
> b/tests/acceptance/boot_firmware.py
> index a41e04936d..05f6728212 100644
> --- a/tests/acceptance/boot_firmware.py
> +++ b/tests/acceptance/boot_firmware.py
> @@ -70,3 +70,55 @@ class BootFirmwareX86(Test):
>              if line + '\n' not in lines:
>                  self.fail('missing: %s' % line)
>          shutil.rmtree(tmpdirname, ignore_errors=True)

Almost every comment on the previous patch applies here of course.
Because of that, it may make sense to generalize some of these patterns
into utility methods and/or, move some common setup code into a setUp()
method.

> +
> +    def test_ovmf(self):
> +        tmpdirname = tempfile.mkdtemp()
> +        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,

For instance, the setting up of the machine type, console and these
common arguments seem to be good candidates to be moved to a common
setUp() method.

> +                         '--bios', '/usr/share/OVMF/OVMF_CODE.fd')

This would be the difference between the arguments of this and the
seabios test, so it could be given by itself here (with the rest on
setUp()).

> +        self.vm.launch()
> +        console = self.vm.console_socket.makefile()
> +
> +        # serial console checks
> +        timeout = time.time() + 15
> +        while True:
> +            msg = console.readline()
> +            if not '\x1b' in msg: # ignore ANSI sequences
> +                serial_logger.debug(msg.strip())
> +            if 'EDK II' in msg:
> +                break
> +            if time.time() > timeout:
> +                self.fail('OVMF failed to boot?')
> +

This seems to be a code block that could become something like:

   def check_boot(self, pattern, timeout):
       ...
       self.fail("Failed to boot")

IMO, a generic fail reason is enough, given that it's associated with
the "test_ovmf".

> +        # debug console checks
> +        expected = [
> +            'SEC: Normal boot',
> +            'S3 support was detected on QEMU',
> +            'Platform PEI Firmware Volume Initialization',
> +            'DXE IPL Entry',
> +            'Installing FVB for EMU Variable support',
> +            'DetectSmbiosVersion: SMBIOS version from QEMU: 0x0208',
> +            'SmbiosCreateTable: Initialize 32-bit entry point structure',
> +            'PlatformBootManagerBeforeConsole',
> +            'OnRootBridgesConnected: root bridges have been connected, '
> +                'installing ACPI tables',
> +            'Found LPC Bridge device',
> +            'PlatformBootManagerAfterConsole',
> +            'EfiBootManagerConnectAll',
> +            '[Bds]Booting EFI Internal Shell',
> +        ]
> +        lines = open(debugcon_path).readlines()
> +        for msg in lines:
> +            debugcon_logger.debug(msg.strip())
> +        for line in expected:
> +            if line + '\r\n' not in lines:
> +                self.fail('missing: %s' % line)
> +        shutil.rmtree(tmpdirname, ignore_errors=True)
> 

And this could also become something like:

   def check_console(self, patterns):
      with open(self.test_logs["debugcon"]) as debugcon:
      content = debugcon.readlines()
      for expected in patterns:
          self.assertIn(expected, content)

Assuming "self.test_logs" is a byproduct of the "self.log_file()"
suggestion for the new feature given previously.