Re: [PATCH 06/22] tests/functional: introduce some helpful decorators

[Thomas Huth <thuth@redhat.com>]

On 29/11/2024 18.31, Daniel P. Berrangé wrote:
> Reduce repeated boilerplate with some helper decorators:
> 
>   @skipIfNotPlatform("x86_64", "aarch64")
> 
>    => Skip unless the build host platform matches
> 
>   @skipIfMissingCommands("mkisofs", "losetup")
> 
>    => Skips unless all listed commands are found in $PATH
> 
>   @skipIfMissingImports("numpy", "cv2")
> 
>    => Skips unless all listed modules can be imported
> 
>   @skipFlakyTest("https://gitlab.com/qemu-project/qemu/-/issues/NNN")
> 
>    => Skips unless env var requests flaky tests with the
>       reason documented in the referenced gitlab bug
> 
>   @skipBigData
> 
>    => Skips unless env var permits tests creating big data files
> 
>   @skipUntrustedTest
> 
>    => Skips unless env var permits tests which are potentially
>       dangerous to the host

That are good ideas! And certainly less error prone than specifying the 
names of the environment variables over and over again.

> diff --git a/tests/functional/qemu_test/__init__.py b/tests/functional/qemu_test/__init__.py
> index 8fddddbe67..7dee3522f2 100644
> --- a/tests/functional/qemu_test/__init__.py
> +++ b/tests/functional/qemu_test/__init__.py
> @@ -13,3 +13,6 @@
>       exec_command, exec_command_and_wait_for_pattern, get_qemu_img, which
>   from .testcase import QemuBaseTest, QemuUserTest, QemuSystemTest
>   from .linuxkernel import LinuxKernelTest
> +from .decorators import skipIfMissingCommands, skipIfNotMachine, \
> +    skipFlakyTest, skipUntrustedTest, skipBigDataTest, \
> +    skipIfMissingImports
> diff --git a/tests/functional/qemu_test/decorators.py b/tests/functional/qemu_test/decorators.py
> new file mode 100644
> index 0000000000..d25fec7b2d
> --- /dev/null
> +++ b/tests/functional/qemu_test/decorators.py
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Decorators useful in functional tests
> +
> +import os
> +import platform
> +from unittest import skipUnless
> +
> +from .cmd import which
> +
> +'''
> +Decorator to skip execution of a test if the list
> +of command binaries is not available in $PATH.
> +Example:
> +
> +  @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 commands(s) "%s" not installed' %

s/commands(s)/command(s)/ ?

> +                      ", ".join(args))
> +
> +'''
> +Decorator to skip execution of a test if the current
> +host machine does not match one of the permitted
> +machines.
> +Example
> +
> +  @skipIfNotMachine("x86_64", "aarch64")
> +'''
> +def skipIfNotMachine(*args):
> +    return skipUnless(lambda: platform.machine() in args,
> +                        'not running on required machine(s) "%s"' %

plural sounds strange here (like all machines would be required at the same 
time), I'd maybe say "not running on one of the required machine(s)" ?

> +                        ", ".join(args))
> +
> +'''
> +Decorator to skip execution of flaky tests, unless
> +the $QEMU_TEST_FLAKY_TESTS env var is set. A bug URL

Since it is the "official" documentation of this decorator, I'd maybe rather 
use the full words: "environment variable" instead of "env var"

> +must be provided that documents the observed failure
> +behaviour, so it can be tracked & re-evaluated in future.
> +
> +Historical tests may be providing "None" as the bug_url
> +but this should not be done for new test.
> +
> +Example:
> +
> +  @skipFlakyTest("https://gitlab.com/qemu-project/qemu/-/issues/NNN")
> +'''
> +def skipFlakyTest(bug_url):
> +    if bug_url is None:
> +        bug_url = "FIXME: reproduce flaky test and file bug report or remove"
> +    return skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'),
> +                      f'Test is unstable: {bug_url}')
> +
> +'''
> +Decorator to skip execution of tests which are likely
> +to execute untrusted commands on the host, or commands
> +which process untrusted code, unles the

s/unles/unless/

> +$QEMU_TEST_ALLOW_UNTRUSTED_CODE env var is set.
> +Example:
> +
> +  @skipUntrustedTest()
> +'''
> +def skipUntrustedTest():
> +    return skipUnless(os.getenv('QEMU_TEST_ALLOW_UNTRUSTED_CODE'),
> +                      'Test runs untrusted code / processes untrusted data')
> +
> +'''
> +Decorator to skip execution of tests which need large
> +data storage on the host, unless the
> +$QEMU_TEST_ALLOW_LARGE_STORAGE env var is set

Maybe we should also provide some direction what is meant with large 
storage. I've seen some tests that are skipped since they create a disk file 
with 128 MiB. And others are always executed though they create a disk file 
with 512 MiB or even more. What would be a good recommendation here?
(My gut feeling is maybe ~ 1 GiB? Or better less?)

> +Example:
> +
> +  @skipBigDataTest()
> +'''
> +def skipBigDataTest():
> +    return skipUnless(os.getenv('QEMU_TEST_ALLOW_LARGE_STORAGE'),
> +                      'Test required large host storage space')

s/required/requires/ ?
(the other decorators use present tense, too)

> +'''
> +Decorator to skip execution of a test if the list
> +of python imports is not available.
> +Example:
> +
> +  @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 imports(s) "%s" not installed' %

s/imports(s)/import(s)/ ?

> +                      ", ".join(args))

  Thomas