Re: [PATCH v4 26/35] tests/functional: Convert the ppc_hv avocado test into a standalone test

[Thomas Huth <thuth@redhat.com>]

On 21/08/2024 11.43, Philippe Mathieu-Daudé wrote:
> On 21/8/24 10:27, Thomas Huth wrote:
>> A straight forward conversion, we just also have to remove the decorator
>> @skipUnless(os.getenv('SPEED')) since all non-trivial functional tests
>> are running in SPEED=thorough mode now. Also make sure that the extracted
>> assets are writable, so that the test does not fail if it gets re-run
>> and there are stale read-only files already around.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   MAINTAINERS                                   |  2 +-
>>   tests/functional/meson.build                  |  2 +
>>   .../test_ppc64_hv.py}                         | 53 ++++++++-----------
>>   3 files changed, 25 insertions(+), 32 deletions(-)
>>   rename tests/{avocado/ppc_hv_tests.py => functional/test_ppc64_hv.py} (85%)
>>   mode change 100644 => 100755
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f01e865025..cca48683ce 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1532,7 +1532,7 @@ F: tests/qtest/libqos/*spapr*
>>   F: tests/qtest/rtas*
>>   F: tests/qtest/libqos/rtas*
>>   F: tests/functional/test_ppc64_pseries.py
>> -F: tests/avocado/ppc_hv_tests.py
>> +F: tests/functional/test_ppc64_hv.py
>>   PowerNV (Non-Virtualized)
>>   M: Cédric Le Goater <clg@kaod.org>
>> diff --git a/tests/functional/meson.build b/tests/functional/meson.build
>> index fba3891e16..f90fb08d6a 100644
>> --- a/tests/functional/meson.build
>> +++ b/tests/functional/meson.build
>> @@ -13,6 +13,7 @@ endif
>>   test_timeouts = {
>>     'netdev_ethtool' : 180,
>>     'ppc_40p' : 240,
>> +  'ppc64_hv' : 1000,
>>     'ppc64_powernv' : 120,
>>     'ppc64_pseries' : 120,
>>     's390x_ccw_virtio' : 180,
>> @@ -63,6 +64,7 @@ tests_ppc_thorough = [
>>   ]
>>   tests_ppc64_thorough = [
>> +  'ppc64_hv',
>>     'ppc64_powernv',
>>     'ppc64_pseries',
>>   ]
>> diff --git a/tests/avocado/ppc_hv_tests.py 
>> b/tests/functional/test_ppc64_hv.py
>> old mode 100644
>> new mode 100755
>> similarity index 85%
>> rename from tests/avocado/ppc_hv_tests.py
>> rename to tests/functional/test_ppc64_hv.py
>> index 0e83bbac71..df59667e07
>> --- a/tests/avocado/ppc_hv_tests.py
>> +++ b/tests/functional/test_ppc64_hv.py
>> @@ -1,3 +1,5 @@
>> +#!/usr/bin/env python3
>> +#
>>   # Tests that specifically try to exercise hypervisor features of the
>>   # target machines. powernv supports the Power hypervisor ISA, and
>>   # pseries supports the nested-HV hypervisor spec.
>> @@ -7,10 +9,10 @@
>>   # This work is licensed under the terms of the GNU GPL, version 2 or
>>   # later.  See the COPYING file in the top-level directory.
>> -from avocado import skipIf, skipUnless
>> -from avocado.utils import archive
>> -from avocado_qemu import QemuSystemTest
>> -from avocado_qemu import wait_for_console_pattern, exec_command
>> +from unittest import skipIf, skipUnless
>> +from qemu_test.utils import archive_extract
>> +from qemu_test import QemuSystemTest, Asset
>> +from qemu_test import wait_for_console_pattern, exec_command
>>   import os
>>   import time
>>   import subprocess
>> @@ -45,8 +47,7 @@ def missing_deps():
>>   # QEMU already installed and use that.
>>   # XXX: The order of these tests seems to matter, see git blame.
>>   @skipIf(missing_deps(), 'dependencies (%s) not installed' % ','.join(deps))
>> -@skipUnless(os.getenv('AVOCADO_ALLOW_LARGE_STORAGE'), 'storage limited')
>> -@skipUnless(os.getenv('SPEED') == 'slow', 'runtime limited')
>> +@skipUnless(os.getenv('QEMU_TEST_ALLOW_LARGE_STORAGE'), 'storage limited')
>>   class HypervisorTest(QemuSystemTest):
>>       timeout = 1000
>> @@ -54,6 +55,11 @@ class HypervisorTest(QemuSystemTest):
>>       panic_message = 'Kernel panic - not syncing'
>>       good_message = 'VFS: Cannot open root device'
>> +    ASSET_ISO = Asset(
>> +        ('https://dl-cdn.alpinelinux.org/alpine/v3.18/'
>> +         'releases/ppc64le/alpine-standard-3.18.4-ppc64le.iso'),
>> +        'c26b8d3e17c2f3f0fed02b4b1296589c2390e6d5548610099af75300edd7b3ff')
>> +
>>       def extract_from_iso(self, iso, path):
>>           """
>>           Extracts a file from an iso file into the test workdir
>> @@ -72,6 +78,7 @@ def extract_from_iso(self, iso, path):
>>               subprocess.run(cmd.split(),
>>                              stdout=subprocess.DEVNULL, 
>> stderr=subprocess.DEVNULL)
>> +        os.chmod(filename, 0o600)
>>           os.chdir(cwd)
>>           # Return complete path to extracted file.  Because callers to
>> @@ -83,16 +90,9 @@ def extract_from_iso(self, iso, path):
>>       def setUp(self):
>>           super().setUp()
>> -        iso_url = 
>> ('https://dl-cdn.alpinelinux.org/alpine/v3.18/releases/ppc64le/alpine-standard-3.18.4-ppc64le.iso')
>> -
>> -        # Alpine use sha256 so I recalculated this myself
>> -        iso_sha256 = 
>> 'c26b8d3e17c2f3f0fed02b4b1296589c2390e6d5548610099af75300edd7b3ff'
>> -        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha256,
>> -                                    algorithm = "sha256")
>> -
>> -        self.iso_path = iso_path
>> -        self.vmlinuz = self.extract_from_iso(iso_path, '/boot/vmlinuz-lts')
>> -        self.initramfs = self.extract_from_iso(iso_path, 
>> '/boot/initramfs-lts')
>> +        self.iso_path = self.ASSET_ISO.fetch()
>> +        self.vmlinuz = self.extract_from_iso(self.iso_path, 
>> '/boot/vmlinuz-lts')
>> +        self.initramfs = self.extract_from_iso(self.iso_path, 
>> '/boot/initramfs-lts')
>>       def do_start_alpine(self):
>>           self.vm.set_console()
> 
> Expanding context:
> 
>             kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE
>             self.vm.add_args("-kernel", self.vmlinuz)
>             self.vm.add_args("-initrd", self.initramfs)
>             self.vm.add_args("-smp", "4", "-m", "2g")
>             self.vm.add_args("-drive", 
> f"file={self.iso_path},format=raw,if=none,id=drive0")
> 
> Should we use ,media=cdrom here too?

I guess that's rather a question for Nicholas (now on CC:)...

Anyway, it should be a separate patch, I think.

  Thomas