Re: [Qemu-devel] [PATCH v2 1/6] tests/acceptance: Add test that runs NetBSD 4.0 installer on PRep/40p

[Cleber Rosa <crosa@redhat.com>]

On Sun, Sep 15, 2019 at 11:19:35PM +0200, Philippe Mathieu-Daudé wrote:
> As of this commit, NetBSD 4.0 is very old. However it is enough to
> test the PRep/40p machine.
> 
> User case from:
> http://mail-index.netbsd.org/port-prep/2017/04/11/msg000112.html
> 
> Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Installers after 4.0 doesn't work anymore, not sure if this is a
> problem from the QEMU model or from NetBSD.
> ---
>  MAINTAINERS                      |  1 +
>  tests/acceptance/ppc_prep_40p.py | 63 ++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>  create mode 100644 tests/acceptance/ppc_prep_40p.py
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50eaf005f4..ce809c7dee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1068,6 +1068,7 @@ F: hw/timer/m48t59-isa.c
>  F: include/hw/isa/pc87312.h
>  F: include/hw/timer/m48t59.h
>  F: pc-bios/ppc_rom.bin
> +F: tests/acceptance/machine_ppc_prep_40p.py
>  
>  sPAPR
>  M: David Gibson <david@gibson.dropbear.id.au>
> diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
> new file mode 100644
> index 0000000000..53f2d2ecf0
> --- /dev/null
> +++ b/tests/acceptance/ppc_prep_40p.py
> @@ -0,0 +1,63 @@
> +# Functional test that boots a PReP/40p machine and checks its serial console.
> +#
> +# Copyright (c) Philippe Mathieu-Daudé <f4bug@amsat.org>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +import os
> +import logging
> +
> +from avocado import skipIf
> +from avocado_qemu import Test
> +
> +
> +class IbmPrep40pMachine(Test):
> +
> +    timeout = 60
> +
> +    # TODO refactor to MachineTest

Your TODO is a clear sign of awareness of the duplicated code that
follows.  The mention of a MachineTest points into the direction that
I can see as the best final solution (that is, test classes that target
specific test scenarions).

But, it may be a more effective refactor strategy, to simply turn the
`wait_for_console_pattern` from a method into a utility function, and
then the discussion of the common test classes (say MachineTest,
GuestTest, MigrationTest) can follow later.

> +    def wait_for_console_pattern(self, success_message, failure_message=None):

Following the previous suggestion, `self` would become something like `test`.

> +        """
> +        Waits for messages to appear on the console, while logging the content
> +

Documented as something like:

  :param test: an Avocado test containing a VM that will have its console
               read and probed for a success or failure message
  :type test: :class:`avocado_qemu.Test`

> +        :param success_message: if this message appears, test succeeds
> +        :param failure_message: if this message appears, test fails
> +        """
> +        console = self.vm.console_socket.makefile()
> +        console_logger = logging.getLogger('console')
> +        while True:
> +            msg = console.readline().strip()
> +            if not msg:
> +                continue
> +            console_logger.debug(msg)
> +            if success_message in msg:
> +                break
> +            if failure_message and failure_message in msg:
> +                fail = 'Failure message found in console: %s' % failure_message
> +                self.fail(fail)
> +
> +    @skipIf(os.getenv('CONTINUOUS_INTEGRATION'), 'Running on Travis-CI')
> +    def test_factory_firmware_and_netbsd(self):
> +        """
> +        :avocado: tags=arch:ppc
> +        :avocado: tags=machine:40p
> +        :avocado: tags=slowness:high

This is certainly a discussion in itself, but I wonder what is your
criteria for definising the slowness level of a test.  FOY, this one
takes me ~17 seconds on my local machine.

> +        """
> +        bios_url = ('ftp://ftp.boulder.ibm.com/rs6000/firmware/'
> +                    '7020-40p/P12H0456.IMG')
> +        bios_hash = '1775face4e6dc27f3a6ed955ef6eb331bf817f03'
> +        bios_path = self.fetch_asset(bios_url, asset_hash=bios_hash)
> +        drive_url = ('https://ftp.netbsd.org/pub/NetBSD/NetBSD-archive/'
> +                     'NetBSD-4.0/prep/installation/floppy/generic_com0.fs')
> +        drive_hash = 'dbcfc09912e71bd5f0d82c7c1ee43082fb596ceb'
> +        drive_path = self.fetch_asset(drive_url, asset_hash=drive_hash)
> +
> +        self.vm.set_machine('40p')

FIY, Avocado 72.0 (due Tomorrow) will have relaxed strictness for tags
values.  That will allow us to represent all current machine type
names in ":avocado: tags=machine:$VALUE" (such as "s390-ccw-virtio").
Then we'll be able to reuse them here, avoiding a bit of boiler plate
code.

> +        self.vm.set_console()
> +        self.vm.add_args('-bios', bios_path,
> +                         '-fda', drive_path)
> +        self.vm.launch()
> +        os_banner = 'NetBSD 4.0 (GENERIC) #0: Sun Dec 16 00:49:40 PST 2007'
> +        self.wait_for_console_pattern(os_banner)
> +        self.wait_for_console_pattern('Model: IBM PPS Model 6015')
> -- 
> 2.20.1
> 

Overall it looks good and works for me.  Let me know what you think of
the wait_for_console_pattern() refactor suggestions.

- Cleber.