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

[PATCH] 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.

skipIfMissingImports also needs to exec() the import statement,
otherwise we always try to import a module called "impname" which
does not exist.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 I've noticed the problem while trying to get the migration test
 through the CI:
 https://gitlab.com/thuth/qemu/-/jobs/8901960783#L100
 ... the OpenSUSE containers apparently lack the "nc" binary ...

 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 df088bc090..7750af7b7d 100644
--- a/tests/functional/qemu_test/decorators.py
+++ b/tests/functional/qemu_test/decorators.py
@@ -16,15 +16,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
@@ -35,9 +34,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
@@ -94,14 +93,13 @@ def skipBigDataTest():
   @skipIfMissingImports("numpy", "cv2")
 '''
 def skipIfMissingImports(*args):
-    def has_imports(importlist):
-        for impname in importlist:
-            try:
-                import 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:
+            exec('import %s' % impname)
+        except ImportError:
+            has_imports = False
+            break
+
+    return skipUnless(has_imports, 'required import(s) "%s" not installed' %
+                                   ", ".join(args))
-- 
2.48.1

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

[Daniel P. Berrangé]

On Tue, Jan 21, 2025 at 07:58:14AM +0100, Thomas Huth wrote:
> 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.

Urgh, I clearly failed to re-test this properly. Originally
I wasn't using skipUnless as a helper, but had implemented
something that looked pretty much like skipUnless and then
refactored it :-(

> 
> skipIfMissingImports also needs to exec() the import statement,
> otherwise we always try to import a module called "impname" which
> does not exist.

Worth doing this as a separate commit.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  I've noticed the problem while trying to get the migration test
>  through the CI:
>  https://gitlab.com/thuth/qemu/-/jobs/8901960783#L100
>  ... the OpenSUSE containers apparently lack the "nc" binary ...
> 
>  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 df088bc090..7750af7b7d 100644
> --- a/tests/functional/qemu_test/decorators.py
> +++ b/tests/functional/qemu_test/decorators.py
> @@ -16,15 +16,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
> @@ -35,9 +34,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
> @@ -94,14 +93,13 @@ def skipBigDataTest():
>    @skipIfMissingImports("numpy", "cv2")
>  '''
>  def skipIfMissingImports(*args):
> -    def has_imports(importlist):
> -        for impname in importlist:
> -            try:
> -                import 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:
> +            exec('import %s' % impname)

I feel like the recommended approach would probably be to use

  importlib.import_module(impname)

> +        except ImportError:
> +            has_imports = False
> +            break
> +
> +    return skipUnless(has_imports, 'required import(s) "%s" not installed' %
> +                                   ", ".join(args))

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