[PATCH v2 0/2] tests/functional: Fix broken decorators

[PATCH v2 0/2] tests/functional: Fix broken decorators

[Thomas Huth]

Many of the new decorators of the functional tests don't work as
expected (and simply always allow to run the tests). Let's fix them!

v2:
- Use importlib.import_module() to check whether we can import a module
- Split the import check into a separate patch

Thomas Huth (2):
  tests/functional/qemu_test/decorators: Fix bad check for imports
  tests/functional: Fix broken decorators with lamda functions

 tests/functional/qemu_test/decorators.py | 45 ++++++++++++------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
2.48.1

[PATCH v2 1/2] tests/functional/qemu_test/decorators: Fix bad check for imports

[Thomas Huth]

skipIfMissingImports should use importlib.import_module() for checking
whether a module with the name stored in the "impname" variable is
available or not, otherwise the code tries to import a module with
the name "impname" instead.
(This bug hasn't been noticed before since there is another issue
with this decorator that will be fixed by the next patch)

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

diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
index df088bc090..08f58f6b40 100644
--- a/tests/functional/qemu_test/decorators.py
+++ b/tests/functional/qemu_test/decorators.py
@@ -2,6 +2,7 @@
 #
 # Decorators useful in functional tests
 
+import importlib
 import os
 import platform
 from unittest import skipUnless
@@ -97,7 +98,7 @@ def skipIfMissingImports(*args):
     def has_imports(importlist):
         for impname in importlist:
             try:
-                import impname
+                importlib.import_module(impname)
             except ImportError:
                 return False
         return True
-- 
2.48.1

[PATCH v2 2/2] tests/functional: Fix broken decorators with lamda functions

[Thomas Huth]

The decorators that use a lambda function are currently broken
and do not properly skip the test if the condition is not met.
Using "return skipUnless(lambda: ...)" does not work as expected.
To fix it, rewrite the decorators without lambda, it's simpler
that way anyway.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/decorators.py | 44 +++++++++++-------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
index 08f58f6b40..3d9c02fd59 100644
--- a/tests/functional/qemu_test/decorators.py
+++ b/tests/functional/qemu_test/decorators.py
@@ -17,15 +17,14 @@
   @skipIfMissingCommands("mkisofs", "losetup")
 '''
 def skipIfMissingCommands(*args):
-    def has_cmds(cmdlist):
-        for cmd in cmdlist:
-            if not which(cmd):
-                return False
-        return True
-
-    return skipUnless(lambda: has_cmds(args),
-                      'required command(s) "%s" not installed' %
-                      ", ".join(args))
+    has_cmds = True
+    for cmd in args:
+         if not which(cmd):
+             has_cmds = False
+             break
+
+    return skipUnless(has_cmds, 'required command(s) "%s" not installed' %
+                                ", ".join(args))
 
 '''
 Decorator to skip execution of a test if the current
@@ -36,9 +35,9 @@ def has_cmds(cmdlist):
   @skipIfNotMachine("x86_64", "aarch64")
 '''
 def skipIfNotMachine(*args):
-    return skipUnless(lambda: platform.machine() in args,
-                        'not running on one of the required machine(s) "%s"' %
-                        ", ".join(args))
+    return skipUnless(platform.machine() in args,
+                      'not running on one of the required machine(s) "%s"' %
+                      ", ".join(args))
 
 '''
 Decorator to skip execution of flaky tests, unless
@@ -95,14 +94,13 @@ def skipBigDataTest():
   @skipIfMissingImports("numpy", "cv2")
 '''
 def skipIfMissingImports(*args):
-    def has_imports(importlist):
-        for impname in importlist:
-            try:
-                importlib.import_module(impname)
-            except ImportError:
-                return False
-        return True
-
-    return skipUnless(lambda: has_imports(args),
-                      'required import(s) "%s" not installed' %
-                      ", ".join(args))
+    has_imports = True
+    for impname in args:
+        try:
+            importlib.import_module(impname)
+        except ImportError:
+            has_imports = False
+            break
+
+    return skipUnless(has_imports, 'required import(s) "%s" not installed' %
+                                   ", ".join(args))
-- 
2.48.1

Re: [PATCH v2 1/2] tests/functional/qemu_test/decorators: Fix bad check for imports

[Daniel P. Berrangé]

On Wed, Jan 22, 2025 at 02:43:13PM +0100, Thomas Huth wrote:
> skipIfMissingImports should use importlib.import_module() for checking
> whether a module with the name stored in the "impname" variable is
> available or not, otherwise the code tries to import a module with
> the name "impname" instead.
> (This bug hasn't been noticed before since there is another issue
> with this decorator that will be fixed by the next patch)
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/functional/qemu_test/decorators.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|