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