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

[Philippe Mathieu-Daudé]

On 9/16/19 6:08 PM, Cleber Rosa wrote:
> 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 <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>> 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 <address@hidden>
>> 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é <address@hidden>
>> +#
>> +# 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.

Yes, I'd like we clean this and make it robust, but for now, the more
tests we have, the better we can see how the common MachineTest should
be. Let's do this in another follow-up series.

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

Ah, I was running this in my --enable-debug --enable-tcg-debug
out-of-tree directory, it takes >2min here.

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

Good news!

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

Thanks, I agree we can do it later :)

Regards,

Phil.