Re: [PATCH v2 05/31] tests/functional: drop 'tesseract_available' helper

[Daniel P . Berrangé]

On Thu, Dec 12, 2024 at 07:57:37AM +0100, Thomas Huth wrote:
> On 11/12/2024 18.26, Daniel P. Berrangé wrote:
> > Platforms we target have new enough tesseract that it suffices to merely
> > check if the binary exists.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/tesseract.py | 12 +-----------
> >   tests/functional/test_m68k_nextcube.py  |  8 +++-----
> >   2 files changed, 4 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tests/functional/qemu_test/tesseract.py 
> > b/tests/functional/qemu_test/tesseract.py
> > index ef1833139d..1b7818090a 100644
> > --- a/tests/functional/qemu_test/tesseract.py
> > +++ b/tests/functional/qemu_test/tesseract.py
> > @@ -7,17 +7,7 @@
> >   import logging
> > -from . import has_cmd, run_cmd
> > -
> > -def tesseract_available(expected_version):
> > -    (has_tesseract, _) = has_cmd('tesseract')
> > -    if not has_tesseract:
> > -        return False
> > -    (stdout, stderr, ret) = run_cmd([ 'tesseract', '--version'])
> > -    if ret:
> > -        return False
> > -    version = stdout.split()[1]
> > -    return int(version.split('.')[0]) >= expected_version
> > +from . import run_cmd
> >   def tesseract_ocr(image_path, tesseract_args=''):
> >       console_logger = logging.getLogger('console')
> > diff --git a/tests/functional/test_m68k_nextcube.py 
> > b/tests/functional/test_m68k_nextcube.py
> > index 0124622c40..1022e8f468 100755
> > --- a/tests/functional/test_m68k_nextcube.py
> > +++ b/tests/functional/test_m68k_nextcube.py
> > @@ -13,7 +13,8 @@
> >   from qemu_test import QemuSystemTest, Asset
> >   from unittest import skipUnless
> > -from qemu_test.tesseract import tesseract_available, tesseract_ocr
> > +from qemu_test import has_cmd
> > +from qemu_test.tesseract import tesseract_ocr
> >   PIL_AVAILABLE = True
> >   try:
> > @@ -53,10 +54,7 @@ def test_bootrom_framebuffer_size(self):
> >           self.assertEqual(width, 1120)
> >           self.assertEqual(height, 832)
> > -    # Tesseract 4 adds a new OCR engine based on LSTM neural networks. The
> > -    # new version is faster and more accurate than version 3. The drawback 
> > is
> > -    # that it is still alpha-level software.
> > -    @skipUnless(tesseract_available(4), 'tesseract OCR tool not available')
> > +    @skipUnless(*has_cmd('tesseract') 'tesseract OCR tool not available')
> 
> The *has_cmd('tesseract') already provides the error message, so you've got
> to drop the 'tesseract OCR tool not available' part now, otherwise this ends
> up in an SyntaxError. You likely didn't notice since it gets replaced later
> anyway, but for bisectability, it would be good to fix it.

Opps, yes, will address it.

> 
> Anyway, this is yet another good example why we should rather get rid of
> has_cmd() ... it's too error prone, I made the same or similar mistake in
> the past already, too.
> 
>  Thomas
> 

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