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

[Daniel P . Berrangé]

On Mon, Dec 02, 2024 at 09:57:27AM +0100, Thomas Huth wrote:
> 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.

Ah good, if we can drop that special case its nice.

> 
> 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)
> ...
> 

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