[PATCH 0/5] tests/functional: a few misc cleanups and fixes

[PATCH 0/5] tests/functional: a few misc cleanups and fixes

[Daniel P. Berrangé]

See individual commit messages for descriptions of the
cleanups/fixes.

Daniel P. Berrangé (5):
  tests/functional: skip test if QEMU_TEST_QEMU_BINARY is not set
  tests/functional: remove unused 'bin_prefix' variable
  tests/functional: set 'qemu_bin' as an object level field
  tests/functional: remove all class level fields
  tests/functional: skip mem addr test on 32-bit hosts

 docs/devel/testing/functional.rst        |  2 +-
 tests/functional/qemu_test/__init__.py   |  2 +-
 tests/functional/qemu_test/decorators.py | 12 ++++++++++++
 tests/functional/qemu_test/testcase.py   | 18 +++++++-----------
 tests/functional/test_mem_addr_space.py  | 17 ++++++++++++++++-
 5 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.47.1

[PATCH 1/5] tests/functional: skip test if QEMU_TEST_QEMU_BINARY is not set

[Daniel P. Berrangé]

If QEMU_TEST_QEMU_BINARY is not set we currently assert in the setUp
function, resulting in a big traceback:

    TAP version 13
    Traceback (most recent call last):
      File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 280, in setUp
        super().setUp('qemu-system-')
        ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
      File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 196, in setUp
        self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
        ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AssertionError: unexpectedly None : QEMU_TEST_QEMU_BINARY must be set

    not ok 1 test_ppc_405.Ppc405Machine.test_ppc_ref405ep
    1..1

For every other test pre-requisite that's missing we will mark the test
as skipped. This does the same for missing QEMU_TEST_QEMU_BINARY, such
that we get

    TAP version 13
    ok 1 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 2 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_noapic_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 3 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 4 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 5 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_noapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 6 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    ok 7 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_vapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
    1..7

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/testcase.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 869f3949fe..94541e8bfb 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -193,7 +193,9 @@ def assets_available(self):
         return True
 
     def setUp(self, bin_prefix):
-        self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
+        if self.qemu_bin is None:
+            self.skipTest("QEMU_TEST_QEMU_BINARY env variable is not set")
+
         self.arch = self.qemu_bin.split('-')[-1]
         self.socketdir = None
 
-- 
2.47.1

[PATCH 2/5] tests/functional: remove unused 'bin_prefix' variable

[Daniel P. Berrangé]

This was copied over from avocado but has not been used in the new
functional tests.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/testcase.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 94541e8bfb..332c782ebc 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -192,7 +192,7 @@ def assets_available(self):
                     return False
         return True
 
-    def setUp(self, bin_prefix):
+    def setUp(self):
         if self.qemu_bin is None:
             self.skipTest("QEMU_TEST_QEMU_BINARY env variable is not set")
 
@@ -256,7 +256,7 @@ def main():
 class QemuUserTest(QemuBaseTest):
 
     def setUp(self):
-        super().setUp('qemu-')
+        super().setUp()
         self._ldpath = []
 
     def add_ldpath(self, ldpath):
@@ -279,7 +279,7 @@ class QemuSystemTest(QemuBaseTest):
     def setUp(self):
         self._vms = {}
 
-        super().setUp('qemu-system-')
+        super().setUp()
 
         console_log = logging.getLogger('console')
         console_log.setLevel(logging.DEBUG)
-- 
2.47.1

[PATCH 3/5] tests/functional: set 'qemu_bin' as an object level field

[Daniel P. Berrangé]

The 'qemu_bin' field is currently set on the class, despite being
accessed as if it were an object instance field with 'self.qemu_bin'.

This is no obvious need to have it as a class field, so move it into
the object instance.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/testing/functional.rst      | 2 +-
 tests/functional/qemu_test/testcase.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
index ecc738922b..bcb5509512 100644
--- a/docs/devel/testing/functional.rst
+++ b/docs/devel/testing/functional.rst
@@ -173,7 +173,7 @@ QEMU binary selection
 ^^^^^^^^^^^^^^^^^^^^^
 
 The QEMU binary used for the ``self.vm`` QEMUMachine instance will
-primarily depend on the value of the ``qemu_bin`` class attribute.
+primarily depend on the value of the ``qemu_bin`` instance attribute.
 If it is not explicitly set by the test code, its default value will
 be the result the QEMU_TEST_QEMU_BINARY environment variable.
 
diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 332c782ebc..574c1942f2 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -33,7 +33,6 @@
 
 class QemuBaseTest(unittest.TestCase):
 
-    qemu_bin = os.getenv('QEMU_TEST_QEMU_BINARY')
     arch = None
 
     workdir = None
@@ -193,6 +192,7 @@ def assets_available(self):
         return True
 
     def setUp(self):
+        self.qemu_bin = os.getenv('QEMU_TEST_QEMU_BINARY')
         if self.qemu_bin is None:
             self.skipTest("QEMU_TEST_QEMU_BINARY env variable is not set")
 
-- 
2.47.1

[PATCH 4/5] tests/functional: remove all class level fields

[Daniel P. Berrangé]

A number of fields are set at the class level on QemuBaseTest, even
though the exact same named field is then set at the object level
later in most cases.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/testcase.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 574c1942f2..531d6393ad 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -33,12 +33,6 @@
 
 class QemuBaseTest(unittest.TestCase):
 
-    arch = None
-
-    workdir = None
-    log = None
-    logdir = None
-
     '''
     @params compressed: filename, Asset, or file-like object to uncompress
     @params format: optional compression format (gzip, lzma)
-- 
2.47.1

[PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Daniel P. Berrangé]

The test_mem_addr_space is validating handling of QEMU with various
memory address settings. All of the test cases are setting 'maxmem'
to a value that exceeds the 32-bit address space, so these must all
be skipped on 32-bit hosts.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/__init__.py   |  2 +-
 tests/functional/qemu_test/decorators.py | 12 ++++++++++++
 tests/functional/test_mem_addr_space.py  | 17 ++++++++++++++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/tests/functional/qemu_test/__init__.py b/tests/functional/qemu_test/__init__.py
index 5c972843a6..7a90311891 100644
--- a/tests/functional/qemu_test/__init__.py
+++ b/tests/functional/qemu_test/__init__.py
@@ -15,6 +15,6 @@
 from .linuxkernel import LinuxKernelTest
 from .decorators import skipIfMissingCommands, skipIfNotMachine, \
     skipFlakyTest, skipUntrustedTest, skipBigDataTest, skipSlowTest, \
-    skipIfMissingImports
+    skipIfMissingImports, skipIf32BitTarget
 from .archive import archive_extract
 from .uncompress import uncompress
diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
index 1651eb739a..d3a8cf0483 100644
--- a/tests/functional/qemu_test/decorators.py
+++ b/tests/functional/qemu_test/decorators.py
@@ -5,6 +5,7 @@
 import importlib
 import os
 import platform
+import sys
 from unittest import skipUnless
 
 from .cmd import which
@@ -118,3 +119,14 @@ def skipIfMissingImports(*args):
 
     return skipUnless(has_imports, 'required import(s) "%s" not installed' %
                                    ", ".join(args))
+
+'''
+Decorator to skip execution of a test on 32-bit targets
+Example:
+
+  @skipIf32BitTarget()
+'''
+def skipIf32BitTarget():
+    enoughBits = sys.maxsize > 2**32
+    return skipUnless(enoughBits,
+                      'Test requires a host with 64-bit address space')
diff --git a/tests/functional/test_mem_addr_space.py b/tests/functional/test_mem_addr_space.py
index bb0cf062ca..3e0215beb3 100755
--- a/tests/functional/test_mem_addr_space.py
+++ b/tests/functional/test_mem_addr_space.py
@@ -10,7 +10,7 @@
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
-from qemu_test import QemuSystemTest
+from qemu_test import QemuSystemTest, skipIf32BitTarget
 import time
 
 class MemAddrCheck(QemuSystemTest):
@@ -22,6 +22,7 @@ class MemAddrCheck(QemuSystemTest):
 
     # first, lets test some 32-bit processors.
     # for all 32-bit cases, pci64_hole_size is 0.
+    @skipIf32BitTarget()
     def test_phybits_low_pse36(self):
         """
         With pse36 feature ON, a processor has 36 bits of addressing. So it can
@@ -49,6 +50,7 @@ def test_phybits_low_pse36(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_low_pae(self):
         """
         With pae feature ON, a processor has 36 bits of addressing. So it can
@@ -66,6 +68,7 @@ def test_phybits_low_pae(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_pentium_pse36(self):
         """
         Setting maxmem to 59.5G and making sure that QEMU can start with the
@@ -82,6 +85,7 @@ def test_phybits_ok_pentium_pse36(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_pentium_pae(self):
         """
         Test is same as above but now with pae cpu feature turned on.
@@ -99,6 +103,7 @@ def test_phybits_ok_pentium_pae(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_pentium2(self):
         """
         Pentium2 has 36 bits of addressing, so its same as pentium
@@ -115,6 +120,7 @@ def test_phybits_ok_pentium2(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_low_nonpse36(self):
         """
         Pentium processor has 32 bits of addressing without pse36 or pae
@@ -135,6 +141,7 @@ def test_phybits_low_nonpse36(self):
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
     # now lets test some 64-bit CPU cases.
+    @skipIf32BitTarget()
     def test_phybits_low_tcg_q35_70_amd(self):
         """
         For q35 7.1 machines and above, there is a HT window that starts at
@@ -161,6 +168,7 @@ def test_phybits_low_tcg_q35_70_amd(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_low_tcg_q35_71_amd(self):
         """
         AMD_HT_START is defined to be at 1012 GiB. So for q35 machines
@@ -181,6 +189,7 @@ def test_phybits_low_tcg_q35_71_amd(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_tcg_q35_70_amd(self):
         """
         Same as q35-7.0 AMD case except that here we check that QEMU can
@@ -197,6 +206,7 @@ def test_phybits_ok_tcg_q35_70_amd(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_tcg_q35_71_amd(self):
         """
         Same as q35-7.1 AMD case except that here we check that QEMU can
@@ -213,6 +223,7 @@ def test_phybits_ok_tcg_q35_71_amd(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_tcg_q35_71_intel(self):
         """
         Same parameters as test_phybits_low_tcg_q35_71_amd() but use
@@ -231,6 +242,7 @@ def test_phybits_ok_tcg_q35_71_intel(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_low_tcg_q35_71_amd_41bits(self):
         """
         AMD processor with 41 bits. Max cpu hw address = 2 TiB.
@@ -255,6 +267,7 @@ def test_phybits_low_tcg_q35_71_amd_41bits(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_tcg_q35_71_amd_41bits(self):
         """
         AMD processor with 41 bits. Max cpu hw address = 2 TiB.
@@ -273,6 +286,7 @@ def test_phybits_ok_tcg_q35_71_amd_41bits(self):
         self.vm.shutdown()
         self.assertNotRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_low_tcg_q35_intel_cxl(self):
         """
         cxl memory window starts after memory device range. Here, we use 1 GiB
@@ -293,6 +307,7 @@ def test_phybits_low_tcg_q35_intel_cxl(self):
         self.assertEqual(self.vm.exitcode(), 1, "QEMU exit code should be 1")
         self.assertRegex(self.vm.get_log(), r'phys-bits too low')
 
+    @skipIf32BitTarget()
     def test_phybits_ok_tcg_q35_intel_cxl(self):
         """
         Same as above but here we do not reserve any cxl memory window. Hence,
-- 
2.47.1

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Philippe Mathieu-Daudé]

On 5/2/25 16:59, Daniel P. Berrangé wrote:
> The test_mem_addr_space is validating handling of QEMU with various
> memory address settings. All of the test cases are setting 'maxmem'
> to a value that exceeds the 32-bit address space, so these must all
> be skipped on 32-bit hosts.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/__init__.py   |  2 +-
>   tests/functional/qemu_test/decorators.py | 12 ++++++++++++
>   tests/functional/test_mem_addr_space.py  | 17 ++++++++++++++++-
>   3 files changed, 29 insertions(+), 2 deletions(-)


> diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
> index 1651eb739a..d3a8cf0483 100644
> --- a/tests/functional/qemu_test/decorators.py
> +++ b/tests/functional/qemu_test/decorators.py
> @@ -5,6 +5,7 @@
>   import importlib
>   import os
>   import platform
> +import sys
>   from unittest import skipUnless
>   
>   from .cmd import which
> @@ -118,3 +119,14 @@ def skipIfMissingImports(*args):
>   
>       return skipUnless(has_imports, 'required import(s) "%s" not installed' %
>                                      ", ".join(args))
> +
> +'''
> +Decorator to skip execution of a test on 32-bit targets
> +Example:
> +
> +  @skipIf32BitTarget()
> +'''
> +def skipIf32BitTarget():
> +    enoughBits = sys.maxsize > 2**32
> +    return skipUnless(enoughBits,
> +                      'Test requires a host with 64-bit address space')

skipIf32BitHost?

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Daniel P. Berrangé]

On Wed, Feb 05, 2025 at 05:40:48PM +0100, Philippe Mathieu-Daudé wrote:
> On 5/2/25 16:59, Daniel P. Berrangé wrote:
> > The test_mem_addr_space is validating handling of QEMU with various
> > memory address settings. All of the test cases are setting 'maxmem'
> > to a value that exceeds the 32-bit address space, so these must all
> > be skipped on 32-bit hosts.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/__init__.py   |  2 +-
> >   tests/functional/qemu_test/decorators.py | 12 ++++++++++++
> >   tests/functional/test_mem_addr_space.py  | 17 ++++++++++++++++-
> >   3 files changed, 29 insertions(+), 2 deletions(-)
> 
> 
> > diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
> > index 1651eb739a..d3a8cf0483 100644
> > --- a/tests/functional/qemu_test/decorators.py
> > +++ b/tests/functional/qemu_test/decorators.py
> > @@ -5,6 +5,7 @@
> >   import importlib
> >   import os
> >   import platform
> > +import sys
> >   from unittest import skipUnless
> >   from .cmd import which
> > @@ -118,3 +119,14 @@ def skipIfMissingImports(*args):
> >       return skipUnless(has_imports, 'required import(s) "%s" not installed' %
> >                                      ", ".join(args))
> > +
> > +'''
> > +Decorator to skip execution of a test on 32-bit targets
> > +Example:
> > +
> > +  @skipIf32BitTarget()
> > +'''
> > +def skipIf32BitTarget():
> > +    enoughBits = sys.maxsize > 2**32
> > +    return skipUnless(enoughBits,
> > +                      'Test requires a host with 64-bit address space')
> 
> skipIf32BitHost?

I don't mind either way.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Philippe Mathieu-Daudé]

On 5/2/25 17:53, Daniel P. Berrangé wrote:
> On Wed, Feb 05, 2025 at 05:40:48PM +0100, Philippe Mathieu-Daudé wrote:
>> On 5/2/25 16:59, Daniel P. Berrangé wrote:
>>> The test_mem_addr_space is validating handling of QEMU with various
>>> memory address settings. All of the test cases are setting 'maxmem'
>>> to a value that exceeds the 32-bit address space, so these must all
>>> be skipped on 32-bit hosts.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    tests/functional/qemu_test/__init__.py   |  2 +-
>>>    tests/functional/qemu_test/decorators.py | 12 ++++++++++++
>>>    tests/functional/test_mem_addr_space.py  | 17 ++++++++++++++++-
>>>    3 files changed, 29 insertions(+), 2 deletions(-)
>>
>>
>>> diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
>>> index 1651eb739a..d3a8cf0483 100644
>>> --- a/tests/functional/qemu_test/decorators.py
>>> +++ b/tests/functional/qemu_test/decorators.py
>>> @@ -5,6 +5,7 @@
>>>    import importlib
>>>    import os
>>>    import platform
>>> +import sys
>>>    from unittest import skipUnless
>>>    from .cmd import which
>>> @@ -118,3 +119,14 @@ def skipIfMissingImports(*args):
>>>        return skipUnless(has_imports, 'required import(s) "%s" not installed' %
>>>                                       ", ".join(args))
>>> +
>>> +'''
>>> +Decorator to skip execution of a test on 32-bit targets

"hosts"

>>> +Example:
>>> +
>>> +  @skipIf32BitTarget()
>>> +'''
>>> +def skipIf32BitTarget():
>>> +    enoughBits = sys.maxsize > 2**32
>>> +    return skipUnless(enoughBits,
>>> +                      'Test requires a host with 64-bit address space')
>>
>> skipIf32BitHost?
> 
> I don't mind either way.

Preferably using skipIf32BitHost to match the error message:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Richard Henderson]

On 2/5/25 07:59, Daniel P. Berrangé wrote:
> +
> +'''
> +Decorator to skip execution of a test on 32-bit targets
> +Example:
> +
> +  @skipIf32BitTarget()
> +'''
> +def skipIf32BitTarget():
> +    enoughBits = sys.maxsize > 2**32

This will work for true 32-bit hosts, and possibly for containers running emulation, but 
it won't work for cross-compilation (x86_64 to i686 or aarch64 to arm).

Perhaps "file qemu-system-foo" | grep "ELF 32-bit" ?
I don't know that we've actually selected the executable at this point though...


r~

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Richard Henderson]

On 2/5/25 08:53, Daniel P. Berrangé wrote:
>>> +Decorator to skip execution of a test on 32-bit targets
>>> +Example:
>>> +
>>> +  @skipIf32BitTarget()
>>> +'''
>>> +def skipIf32BitTarget():
>>> +    enoughBits = sys.maxsize > 2**32
>>> +    return skipUnless(enoughBits,
>>> +                      'Test requires a host with 64-bit address space')
>>
>> skipIf32BitHost?
> 
> I don't mind either way.

Definitely host.


r~

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Daniel P. Berrangé]

On Wed, Feb 05, 2025 at 10:24:08AM -0800, Richard Henderson wrote:
> On 2/5/25 07:59, Daniel P. Berrangé wrote:
> > +
> > +'''
> > +Decorator to skip execution of a test on 32-bit targets
> > +Example:
> > +
> > +  @skipIf32BitTarget()
> > +'''
> > +def skipIf32BitTarget():
> > +    enoughBits = sys.maxsize > 2**32
> 
> This will work for true 32-bit hosts, and possibly for containers running
> emulation, but it won't work for cross-compilation (x86_64 to i686 or
> aarch64 to arm).

If we've cross compiled qemu-system-XXXX then we won't be able to
execute any functional tests for those binaries, so is it actually
a problem ? 


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Richard Henderson]

On 2/5/25 10:47, Daniel P. Berrangé wrote:
> On Wed, Feb 05, 2025 at 10:24:08AM -0800, Richard Henderson wrote:
>> On 2/5/25 07:59, Daniel P. Berrangé wrote:
>>> +
>>> +'''
>>> +Decorator to skip execution of a test on 32-bit targets
>>> +Example:
>>> +
>>> +  @skipIf32BitTarget()
>>> +'''
>>> +def skipIf32BitTarget():
>>> +    enoughBits = sys.maxsize > 2**32
>>
>> This will work for true 32-bit hosts, and possibly for containers running
>> emulation, but it won't work for cross-compilation (x86_64 to i686 or
>> aarch64 to arm).
> 
> If we've cross compiled qemu-system-XXXX then we won't be able to
> execute any functional tests for those binaries, so is it actually
> a problem ?

Absolutely we can execute those binaries, for the specific examples above.


r~

Re: [PATCH 1/5] tests/functional: skip test if QEMU_TEST_QEMU_BINARY is not set

[Thomas Huth]

On 05/02/2025 16.59, Daniel P. Berrangé wrote:
> If QEMU_TEST_QEMU_BINARY is not set we currently assert in the setUp
> function, resulting in a big traceback:
> 
>      TAP version 13
>      Traceback (most recent call last):
>        File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 280, in setUp
>          super().setUp('qemu-system-')
>          ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
>        File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 196, in setUp
>          self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
>          ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      AssertionError: unexpectedly None : QEMU_TEST_QEMU_BINARY must be set
> 
>      not ok 1 test_ppc_405.Ppc405Machine.test_ppc_ref405ep
>      1..1
> 
> For every other test pre-requisite that's missing we will mark the test
> as skipped. This does the same for missing QEMU_TEST_QEMU_BINARY, such
> that we get
> 
>      TAP version 13
>      ok 1 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 2 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_noapic_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 3 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 4 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 5 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_noapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 6 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      ok 7 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_vapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
>      1..7

Not sure whether this is the right approach, since a missing 
QEMU_TEST_QEMU_BINARY is a real error, and if we just skip, then the problem 
might go unnoticed if the user does not look closely.

But to ease the situation: We could maybe add some auto-detection logic that 
tries to guess the right qemu-system-$TARGET by looking at the file name of 
the test and/or the test function name? We already encode the target 
architecture in most of these... WDYT?

  Thomas

Re: [PATCH 2/5] tests/functional: remove unused 'bin_prefix' variable

[Thomas Huth]

On 05/02/2025 16.59, Daniel P. Berrangé wrote:
> This was copied over from avocado but has not been used in the new
> functional tests.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/testcase.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
> index 94541e8bfb..332c782ebc 100644
> --- a/tests/functional/qemu_test/testcase.py
> +++ b/tests/functional/qemu_test/testcase.py
> @@ -192,7 +192,7 @@ def assets_available(self):
>                       return False
>           return True
>   
> -    def setUp(self, bin_prefix):
> +    def setUp(self):
>           if self.qemu_bin is None:
>               self.skipTest("QEMU_TEST_QEMU_BINARY env variable is not set")
>   
> @@ -256,7 +256,7 @@ def main():
>   class QemuUserTest(QemuBaseTest):
>   
>       def setUp(self):
> -        super().setUp('qemu-')
> +        super().setUp()
>           self._ldpath = []
>   
>       def add_ldpath(self, ldpath):
> @@ -279,7 +279,7 @@ class QemuSystemTest(QemuBaseTest):
>       def setUp(self):
>           self._vms = {}
>   
> -        super().setUp('qemu-system-')
> +        super().setUp()

We might still need it in case we try to add auto-detection of the QEMU 
binary again, but yes, for the time being:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/5] tests/functional: set 'qemu_bin' as an object level field

[Thomas Huth]

On 05/02/2025 16.59, Daniel P. Berrangé wrote:
> The 'qemu_bin' field is currently set on the class, despite being
> accessed as if it were an object instance field with 'self.qemu_bin'.
> 
> This is no obvious need to have it as a class field, so move it into
> the object instance.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   docs/devel/testing/functional.rst      | 2 +-
>   tests/functional/qemu_test/testcase.py | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 4/5] tests/functional: remove all class level fields

[Thomas Huth]

On 05/02/2025 16.59, Daniel P. Berrangé wrote:
> A number of fields are set at the class level on QemuBaseTest, even
> though the exact same named field is then set at the object level
> later in most cases.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/testcase.py | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
> index 574c1942f2..531d6393ad 100644
> --- a/tests/functional/qemu_test/testcase.py
> +++ b/tests/functional/qemu_test/testcase.py
> @@ -33,12 +33,6 @@
>   
>   class QemuBaseTest(unittest.TestCase):
>   
> -    arch = None
> -
> -    workdir = None
> -    log = None
> -    logdir = None

That's what you get when a Python ignorant like me tries to write object 
oriented Python code ;-) Thanks for cleaning up my mess!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Thomas Huth]

On 05/02/2025 19.25, Richard Henderson wrote:
> On 2/5/25 08:53, Daniel P. Berrangé wrote:
>>>> +Decorator to skip execution of a test on 32-bit targets
>>>> +Example:
>>>> +
>>>> +  @skipIf32BitTarget()
>>>> +'''
>>>> +def skipIf32BitTarget():
>>>> +    enoughBits = sys.maxsize > 2**32
>>>> +    return skipUnless(enoughBits,
>>>> +                      'Test requires a host with 64-bit address space')
>>>
>>> skipIf32BitHost?
>>
>> I don't mind either way.
> 
> Definitely host.

+1 for host. Otherwise this gets confused with "qemu-system-i386" etc.

  Thomas

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Daniel P. Berrangé]

On Wed, Feb 05, 2025 at 10:24:08AM -0800, Richard Henderson wrote:
> On 2/5/25 07:59, Daniel P. Berrangé wrote:
> > +
> > +'''
> > +Decorator to skip execution of a test on 32-bit targets
> > +Example:
> > +
> > +  @skipIf32BitTarget()
> > +'''
> > +def skipIf32BitTarget():
> > +    enoughBits = sys.maxsize > 2**32
> 
> This will work for true 32-bit hosts, and possibly for containers running
> emulation, but it won't work for cross-compilation (x86_64 to i686 or
> aarch64 to arm).
> 
> Perhaps "file qemu-system-foo" | grep "ELF 32-bit" ?
> I don't know that we've actually selected the executable at this point though...

The QEMU_TEST_QEMU_BINARY env variable exists at all times, though
grepping for ELF format feels a bit icky.

We also know the location of the build directory, so was wondering if
anything there tells us whether the host target is 64-bit, but it
appears not be the case.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/5] tests/functional: skip mem addr test on 32-bit hosts

[Thomas Huth]

On 06/02/2025 10.35, Daniel P. Berrangé wrote:
> On Wed, Feb 05, 2025 at 10:24:08AM -0800, Richard Henderson wrote:
>> On 2/5/25 07:59, Daniel P. Berrangé wrote:
>>> +
>>> +'''
>>> +Decorator to skip execution of a test on 32-bit targets
>>> +Example:
>>> +
>>> +  @skipIf32BitTarget()
>>> +'''
>>> +def skipIf32BitTarget():
>>> +    enoughBits = sys.maxsize > 2**32
>>
>> This will work for true 32-bit hosts, and possibly for containers running
>> emulation, but it won't work for cross-compilation (x86_64 to i686 or
>> aarch64 to arm).
>>
>> Perhaps "file qemu-system-foo" | grep "ELF 32-bit" ?
>> I don't know that we've actually selected the executable at this point though...
> 
> The QEMU_TEST_QEMU_BINARY env variable exists at all times, though
> grepping for ELF format feels a bit icky.
> 
> We also know the location of the build directory, so was wondering if
> anything there tells us whether the host target is 64-bit, but it
> appears not be the case.

Maybe it's sufficient to wait for Richard's patch to get merged:

  https://lore.kernel.org/qemu-devel/20250204215359.1238808-11-richard.henderson@linaro.org/

?

  Thomas

Re: [PATCH 1/5] tests/functional: skip test if QEMU_TEST_QEMU_BINARY is not set

[Daniel P. Berrangé]

On Thu, Feb 06, 2025 at 10:01:45AM +0100, Thomas Huth wrote:
> On 05/02/2025 16.59, Daniel P. Berrangé wrote:
> > If QEMU_TEST_QEMU_BINARY is not set we currently assert in the setUp
> > function, resulting in a big traceback:
> > 
> >      TAP version 13
> >      Traceback (most recent call last):
> >        File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 280, in setUp
> >          super().setUp('qemu-system-')
> >          ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
> >        File "/var/home/berrange/src/virt/qemu/tests/functional/qemu_test/testcase.py", line 196, in setUp
> >          self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
> >          ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      AssertionError: unexpectedly None : QEMU_TEST_QEMU_BINARY must be set
> > 
> >      not ok 1 test_ppc_405.Ppc405Machine.test_ppc_ref405ep
> >      1..1
> > 
> > For every other test pre-requisite that's missing we will mark the test
> > as skipped. This does the same for missing QEMU_TEST_QEMU_BINARY, such
> > that we get
> > 
> >      TAP version 13
> >      ok 1 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 2 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_noapic_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 3 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 4 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 5 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_noapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 6 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_novector_nomsi # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      ok 7 test_x86_64_kvm_xen.KVMXenGuest.test_kvm_xen_guest_vapic # SKIP QEMU_TEST_QEMU_BINARY env variable is not set
> >      1..7
> 
> Not sure whether this is the right approach, since a missing
> QEMU_TEST_QEMU_BINARY is a real error, and if we just skip, then the problem
> might go unnoticed if the user does not look closely.
> 
> But to ease the situation: We could maybe add some auto-detection logic that
> tries to guess the right qemu-system-$TARGET by looking at the file name of
> the test and/or the test function name? We already encode the target
> architecture in most of these... WDYT?

When the user provides a binary, a single binary applies to all tests,
so tests that don't match the binary get skipped. I think it would be
a bit wierd to auto-select a different binary per test.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|