Re: [PATCH 07/22] tests/functional: switch to new test skip decorators

[Thomas Huth <thuth@redhat.com>]

On 29/11/2024 18.31, Daniel P. Berrangé wrote:
> This ensures consistency of behaviour across all the tests, and requires
> that we provide gitlab bug links when marking a test to be skipped due
> to unreliability.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> diff --git a/tests/functional/test_arm_aspeed.py b/tests/functional/test_arm_aspeed.py
> index 068740a813..db872ff05e 100755
> --- a/tests/functional/test_arm_aspeed.py
> +++ b/tests/functional/test_arm_aspeed.py
> @@ -13,7 +13,9 @@
>   
>   from qemu_test import (LinuxKernelTest, Asset,
>                          exec_command_and_wait_for_pattern,
> -                       interrupt_interactive_console_until_pattern, has_cmd)
> +                       interrupt_interactive_console_until_pattern,
> +                       skipIfMissingCommands,
> +)

In the other files, you placed the final ")" at the end of the previous line 
instead?

> diff --git a/tests/functional/test_m68k_nextcube.py b/tests/functional/test_m68k_nextcube.py
> index 0124622c40..82d3d335d0 100755
> --- a/tests/functional/test_m68k_nextcube.py
> +++ b/tests/functional/test_m68k_nextcube.py
> @@ -10,16 +10,9 @@
>   import os
>   import time
>   
> -from qemu_test import QemuSystemTest, Asset
> -from unittest import skipUnless
> -
> +from qemu_test import QemuSystemTest, Asset, skipIfMissingImports
>   from qemu_test.tesseract import tesseract_available, tesseract_ocr
> -
> -PIL_AVAILABLE = True
> -try:
> -    from PIL import Image
> -except ImportError:
> -    PIL_AVAILABLE = False
> +from unittest import skipUnless

I think you could also replace the other skipUnless() in this file nowadays: 
The version check here was only useful in the days when most distros still 
shipped Tesseract v3, but these days are gone, we don't support any of those 
distros anymore. So I think it should be fine to use skipIfMissingCommands 
here now instead.

Anyway, I'm also fine if we keep it for now (we still can adjust it later), 
so with at least the ")" nit fixed:

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

>   class NextCubeMachine(QemuSystemTest):
> @@ -43,12 +36,13 @@ def check_bootrom_framebuffer(self, screenshot_path):
>           self.vm.cmd('human-monitor-command',
>                       command_line='screendump %s' % screenshot_path)
>   
> -    @skipUnless(PIL_AVAILABLE, 'Python PIL not installed')
> +    @skipIfMissingImports("PIL")
>       def test_bootrom_framebuffer_size(self):
>           self.set_machine('next-cube')
>           screenshot_path = os.path.join(self.workdir, "dump.ppm")
>           self.check_bootrom_framebuffer(screenshot_path)
>   
> +        from PIL import Image
>           width, height = Image.open(screenshot_path).size
>           self.assertEqual(width, 1120)
>           self.assertEqual(height, 832)
...