[PATCH 0/4] Silence some pylint warnings in tests/functional/qemu_test/

[PATCH 0/4] Silence some pylint warnings in tests/functional/qemu_test/

[Thomas Huth]

While we already check the target folders in the functional testing
framework with pylint, the folder with the core classes of the
functional testing framework is not clean for pylint yet.
Here are some patches to tackle the warnings in three of the files at
least.

Thomas Huth (4):
  tests/functional/qemu_test: Silence warnings from pylint in
    tesseract.py
  tests/functional/qemu_test: Silence (most) warnings from pylint in
    asset.py
  tests/functional/qemu_test: Split huge fetch() function in asset.py
  tests/functional/qemu_test: Silence warnings from pylint in config.py

 tests/functional/qemu_test/asset.py     | 94 ++++++++++++++++---------
 tests/functional/qemu_test/config.py    | 11 +--
 tests/functional/qemu_test/tesseract.py | 11 ++-
 3 files changed, 76 insertions(+), 40 deletions(-)

-- 
2.53.0

[PATCH 1/4] tests/functional/qemu_test: Silence warnings from pylint in tesseract.py

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Pylint complains:

 tesseract.py:1:0: C0114: Missing module docstring (missing-module-docstring)
 tesseract.py:12:0: C0116: Missing function or method docstring (missing-function-docstring)
 tesseract.py:15:11: W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check)
 tesseract.py:12:30: W0613: Unused argument 'tesseract_args' (unused-argument)

Thus add the missing bits and remove the unused tesseract_args argument.
While we're at it, also add a SPDX identifier instead of the weird three
dots at the beginning of the file.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/tesseract.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/functional/qemu_test/tesseract.py b/tests/functional/qemu_test/tesseract.py
index ede6c6501e2..1f955d32b76 100644
--- a/tests/functional/qemu_test/tesseract.py
+++ b/tests/functional/qemu_test/tesseract.py
@@ -1,19 +1,24 @@
-# ...
+# SPDX-License-Identifier: GPL-2.0-or-later
 #
 # Copyright (c) 2019 Philippe Mathieu-Daudé <f4bug@amsat.org>
 #
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later. See the COPYING file in the top-level directory.
+'''
+Tesseract is an program for doing Optical Character Recognition (OCR),
+which can be used in the tests to extract text from screenshots.
+'''
 
 import logging
 from subprocess import run
 
 
-def tesseract_ocr(image_path, tesseract_args=''):
+def tesseract_ocr(image_path):
+    ''' Run the tesseract OCR to extract text from a screenshot '''
     console_logger = logging.getLogger('console')
     console_logger.debug(image_path)
     proc = run(['tesseract', image_path, 'stdout'],
-               capture_output=True, encoding='utf8')
+               capture_output=True, encoding='utf8', check=False)
     if proc.returncode:
         return None
     lines = []
-- 
2.53.0

[PATCH 2/4] tests/functional/qemu_test: Silence (most) warnings from pylint in asset.py

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Pylint currently complains:

 asset.py:1:0: C0114: Missing module docstring (missing-module-docstring)
 asset.py:21:0: C0115: Missing class docstring (missing-class-docstring)
 asset.py:28:15: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:34:0: C0115: Missing class docstring (missing-class-docstring)
 asset.py:49:15: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:73:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:80:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:83:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:125:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:181:43: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:190:39: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:201:39: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:213:15: W0718: Catching too general exception Exception (broad-exception-caught)
 asset.py:218:35: C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
 asset.py:125:4: R0912: Too many branches (16/12) (too-many-branches)
 asset.py:125:4: R0915: Too many statements (64/50) (too-many-statements)
 asset.py:228:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:249:4: C0116: Missing function or method docstring (missing-function-docstring)
 asset.py:257:4: C0116: Missing function or method docstring (missing-function-docstring)

Fix all the warnings except for the R0912 and R0915 which will be tackled
in a later commit.
And while we're at it, also add a proper SPDX license identifier.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/asset.py | 53 +++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index 45a2e01e2e6..1cd03c2a9a9 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -1,9 +1,12 @@
-# Test utilities for fetching & caching assets
+# SPDX-License-Identifier: GPL-2.0-or-later
 #
 # Copyright 2024 Red Hat, Inc.
 #
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later.  See the COPYING file in the top-level directory.
+'''
+Test utilities for fetching & caching assets
+'''
 
 import hashlib
 import logging
@@ -18,21 +21,27 @@
 from shutil import copyfileobj
 from urllib.error import HTTPError, URLError
 
+
 class AssetError(Exception):
+    '''This exception will be raised if an asset is not usable'''
     def __init__(self, asset, msg, transient=False):
         self.url = asset.url
         self.msg = msg
         self.transient = transient
 
     def __str__(self):
-        return "%s: %s" % (self.url, self.msg)
+        return f"{self.url}: {self.msg}"
 
-# Instances of this class must be declared as class level variables
-# starting with a name "ASSET_". This enables the pre-caching logic
-# to easily find all referenced assets and download them prior to
-# execution of the tests.
-class Asset:
 
+class Asset:
+    '''
+    This class is used to represent an asset that gets downloaded from
+    the internet and will be stored in the local asset cache.
+    Instances of this class must be declared as class level variables
+    starting with a name "ASSET_". This enables the pre-caching logic
+    to easily find all referenced assets and download them prior to
+    execution of the tests.
+    '''
     def __init__(self, url, hashsum):
         self.url = url
         self.hash = hashsum
@@ -46,8 +55,7 @@ def __init__(self, url, hashsum):
         self.log = logging.getLogger('qemu-test')
 
     def __repr__(self):
-        return "Asset: url=%s hash=%s cache=%s" % (
-            self.url, self.hash, self.cache_file)
+        return f"Asset: url={self.url} hash={self.hash} cache={self.cache_file}"
 
     def __str__(self):
         return str(self.cache_file)
@@ -71,6 +79,7 @@ def _check(self, cache_file):
         return self.hash == hl.hexdigest()
 
     def valid(self):
+        '''Check whether the file exists in the cache and has the right hash'''
         if os.getenv("QEMU_TEST_REFRESH_CACHE", None) is not None:
             self.log.info("Force refresh of asset %s", self.url)
             return False
@@ -78,9 +87,11 @@ def valid(self):
         return self.cache_file.exists() and self._check(self.cache_file)
 
     def fetchable(self):
+        '''Check whether we are allowed to download assets from the internet'''
         return not os.environ.get("QEMU_TEST_NO_DOWNLOAD", False)
 
     def available(self):
+        '''Check whether the asset is either in the cache or fetchable'''
         return self.valid() or self.fetchable()
 
     def _wait_for_other_download(self, tmp_cache_file):
@@ -123,6 +134,7 @@ def _save_time_stamp(self):
         self.cache_file.with_suffix(".stamp").write_text(f"{int(time.time())}")
 
     def fetch(self):
+        '''Download the asset from the internet'''
         if not self.cache_dir.exists():
             self.cache_dir.mkdir(parents=True, exist_ok=True)
 
@@ -179,7 +191,7 @@ def fetch(self):
                 # server or networking problem
                 if e.code == 404:
                     raise AssetError(self, "Unable to download: "
-                                     "HTTP error %d" % e.code) from e
+                                     f"HTTP error {e.code}") from e
                 continue
             except URLError as e:
                 # This is typically a network/service level error
@@ -187,8 +199,9 @@ def fetch(self):
                 tmp_cache_file.unlink()
                 self.log.error("Unable to download %s: URL error %s",
                                self.url, e.reason)
-                raise AssetError(self, "Unable to download: URL error %s" %
-                                 e.reason, transient=True) from e
+                raise AssetError(self,
+                                 f"Unable to download: URL error{e.reason}",
+                                 transient=True) from e
             except ConnectionError as e:
                 # A socket connection failure, such as dropped conn
                 # or refused conn
@@ -198,7 +211,7 @@ def fetch(self):
                 continue
             except Exception as e:
                 tmp_cache_file.unlink()
-                raise AssetError(self, "Unable to download: %s" % e,
+                raise AssetError(self, f"Unable to download: {e}",
                                  transient=True) from e
 
         if not os.path.exists(tmp_cache_file):
@@ -210,12 +223,12 @@ def fetch(self):
                         self.url.encode('utf8'))
             os.setxattr(str(tmp_cache_file), "user.qemu-asset-hash",
                         self.hash.encode('utf8'))
-        except Exception as e:
+        except OSError as e:
             self.log.debug("Unable to set xattr on %s: %s", tmp_cache_file, e)
 
         if not self._check(tmp_cache_file):
             tmp_cache_file.unlink()
-            raise AssetError(self, "Hash does not match %s" % self.hash)
+            raise AssetError(self, f"Hash does not match {self.hash}")
         tmp_cache_file.replace(self.cache_file)
         self._save_time_stamp()
         # Remove write perms to stop tests accidentally modifying them
@@ -226,6 +239,10 @@ def fetch(self):
 
     @staticmethod
     def precache_test(test):
+        '''
+        Look for variables starting with "ASSET_" and try to fetch the asset
+        that is specified there.
+        '''
         log = logging.getLogger('qemu-test')
         log.setLevel(logging.DEBUG)
         handler = logging.StreamHandler(sys.stdout)
@@ -247,6 +264,9 @@ def precache_test(test):
 
     @staticmethod
     def precache_suite(suite):
+        '''
+        Iterate through all tests/suites in a suite and precache their assets
+        '''
         for test in suite:
             if isinstance(test, unittest.TestSuite):
                 Asset.precache_suite(test)
@@ -255,6 +275,9 @@ def precache_suite(suite):
 
     @staticmethod
     def precache_suites(path, cache_tstamp):
+        '''
+        Get the available test suite and precache their assets
+        '''
         loader = unittest.loader.defaultTestLoader
         tests = loader.loadTestsFromNames([path], None)
 
-- 
2.53.0

[PATCH 3/4] tests/functional/qemu_test: Split huge fetch() function in asset.py

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

The fetch() function has become really huge and pylint complains about
that. Extract the internal retry-three-times-download loop into a
separate function to make it a little bit more readable and to make
pylint happy about this file again.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/asset.py | 43 ++++++++++++++++-------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index 1cd03c2a9a9..51a434b2b70 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -133,24 +133,7 @@ def _save_time_stamp(self):
         '''
         self.cache_file.with_suffix(".stamp").write_text(f"{int(time.time())}")
 
-    def fetch(self):
-        '''Download the asset from the internet'''
-        if not self.cache_dir.exists():
-            self.cache_dir.mkdir(parents=True, exist_ok=True)
-
-        if self.valid():
-            self.log.debug("Using cached asset %s for %s",
-                           self.cache_file, self.url)
-            self._save_time_stamp()
-            return str(self.cache_file)
-
-        if not self.fetchable():
-            raise AssetError(self,
-                             "Asset cache is invalid and downloads disabled")
-
-        self.log.info("Downloading %s to %s...", self.url, self.cache_file)
-        tmp_cache_file = self.cache_file.with_suffix(".download")
-
+    def _try_to_fetch(self, tmp_cache_file):
         for _retries in range(3):
             try:
                 with tmp_cache_file.open("xb") as dst:
@@ -176,7 +159,7 @@ def fetch(self):
                                "waiting for other thread to finish...",
                                tmp_cache_file)
                 if self._wait_for_other_download(tmp_cache_file):
-                    return str(self.cache_file)
+                    return True
                 self.log.debug("%s seems to be stale, "
                                "deleting and retrying download...",
                                tmp_cache_file)
@@ -213,6 +196,28 @@ def fetch(self):
                 tmp_cache_file.unlink()
                 raise AssetError(self, f"Unable to download: {e}",
                                  transient=True) from e
+        return False
+
+    def fetch(self):
+        '''Download the asset from the internet'''
+        if not self.cache_dir.exists():
+            self.cache_dir.mkdir(parents=True, exist_ok=True)
+
+        if self.valid():
+            self.log.debug("Using cached asset %s for %s",
+                           self.cache_file, self.url)
+            self._save_time_stamp()
+            return str(self.cache_file)
+
+        if not self.fetchable():
+            raise AssetError(self,
+                             "Asset cache is invalid and downloads disabled")
+
+        self.log.info("Downloading %s to %s...", self.url, self.cache_file)
+        tmp_cache_file = self.cache_file.with_suffix(".download")
+
+        if self._try_to_fetch(tmp_cache_file):
+            return str(self.cache_file)
 
         if not os.path.exists(tmp_cache_file):
             raise AssetError(self, "Download retries exceeded", transient=True)
-- 
2.53.0

[PATCH 4/4] tests/functional/qemu_test: Silence warnings from pylint in config.py

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Pylint complains here:

 config.py:1:0: C0114: Missing module docstring (missing-module-docstring)
 config.py:28:4: W0719: Raising too general exception: Exception (broad-exception-raised)

Add a module description and replace the general Exception to fix this.
And while we're at it, and since we've got a proper module description
string now, also replace the copy-n-pasted comment at the top of the file
with a proper SPDX identifier.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/config.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/functional/qemu_test/config.py b/tests/functional/qemu_test/config.py
index e0893f630ee..5d44b6fa4c0 100644
--- a/tests/functional/qemu_test/config.py
+++ b/tests/functional/qemu_test/config.py
@@ -1,4 +1,4 @@
-# Test class and utilities for functional tests
+# SPDX-License-Identifier: GPL-2.0-or-later
 #
 # Copyright 2018, 2024 Red Hat, Inc.
 #
@@ -10,6 +10,9 @@
 #
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later.  See the COPYING file in the top-level directory.
+'''
+Functions related to the configuration of the tests and of the host system
+'''
 
 import os
 from pathlib import Path
@@ -25,9 +28,9 @@ def _build_dir():
     if root is not None:
         return Path(root)
 
-    raise Exception("Missing MESON_BUILD_ROOT environment variable. " +
-                    "Please use the '<BUILD-DIR>/run' script if invoking " +
-                    "directly instead of via make/meson")
+    raise RuntimeError("Missing MESON_BUILD_ROOT environment variable. " +
+                       "Please use the '<BUILD-DIR>/run' script if invoking " +
+                       "directly instead of via make/meson")
 
 BUILD_DIR = _build_dir()
 
-- 
2.53.0

Re: [PATCH 1/4] tests/functional/qemu_test: Silence warnings from pylint in tesseract.py

[Philippe Mathieu-Daudé]

On 24/3/26 17:35, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Pylint complains:
> 
>   tesseract.py:1:0: C0114: Missing module docstring (missing-module-docstring)
>   tesseract.py:12:0: C0116: Missing function or method docstring (missing-function-docstring)
>   tesseract.py:15:11: W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check)
>   tesseract.py:12:30: W0613: Unused argument 'tesseract_args' (unused-argument)
> 
> Thus add the missing bits and remove the unused tesseract_args argument.
> While we're at it, also add a SPDX identifier instead of the weird three
> dots at the beginning of the file.

I'm OK to remove the boilerplate if possible.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/functional/qemu_test/tesseract.py | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/tesseract.py b/tests/functional/qemu_test/tesseract.py
> index ede6c6501e2..1f955d32b76 100644
> --- a/tests/functional/qemu_test/tesseract.py
> +++ b/tests/functional/qemu_test/tesseract.py
> @@ -1,19 +1,24 @@
> -# ...
> +# SPDX-License-Identifier: GPL-2.0-or-later
>   #
>   # Copyright (c) 2019 Philippe Mathieu-Daudé <f4bug@amsat.org>
>   #
>   # This work is licensed under the terms of the GNU GPL, version 2 or
>   # later. See the COPYING file in the top-level directory.
> +'''
> +Tesseract is an program for doing Optical Character Recognition (OCR),
> +which can be used in the tests to extract text from screenshots.
> +'''
>   
>   import logging
>   from subprocess import run
>   
>   
> -def tesseract_ocr(image_path, tesseract_args=''):
> +def tesseract_ocr(image_path):
> +    ''' Run the tesseract OCR to extract text from a screenshot '''
>       console_logger = logging.getLogger('console')
>       console_logger.debug(image_path)
>       proc = run(['tesseract', image_path, 'stdout'],
> -               capture_output=True, encoding='utf8')
> +               capture_output=True, encoding='utf8', check=False)
>       if proc.returncode:
>           return None
>       lines = []

Re: [PATCH 3/4] tests/functional/qemu_test: Split huge fetch() function in asset.py

[Philippe Mathieu-Daudé]

On 24/3/26 17:35, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The fetch() function has become really huge and pylint complains about
> that. Extract the internal retry-three-times-download loop into a
> separate function to make it a little bit more readable and to make
> pylint happy about this file again.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/functional/qemu_test/asset.py | 43 ++++++++++++++++-------------
>   1 file changed, 24 insertions(+), 19 deletions(-)


> +    def _try_to_fetch(self, tmp_cache_file):
>           for _retries in range(3):

While here, pass the number as argument to the new function?

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    def fetch(self):
> +        '''Download the asset from the internet'''
> +        if not self.cache_dir.exists():
> +            self.cache_dir.mkdir(parents=True, exist_ok=True)
> +
> +        if self.valid():
> +            self.log.debug("Using cached asset %s for %s",
> +                           self.cache_file, self.url)
> +            self._save_time_stamp()
> +            return str(self.cache_file)
> +
> +        if not self.fetchable():
> +            raise AssetError(self,
> +                             "Asset cache is invalid and downloads disabled")
> +
> +        self.log.info("Downloading %s to %s...", self.url, self.cache_file)
> +        tmp_cache_file = self.cache_file.with_suffix(".download")
> +
> +        if self._try_to_fetch(tmp_cache_file):
> +            return str(self.cache_file)
>   
>           if not os.path.exists(tmp_cache_file):
>               raise AssetError(self, "Download retries exceeded", transient=True)

Re: [PATCH 4/4] tests/functional/qemu_test: Silence warnings from pylint in config.py

[Philippe Mathieu-Daudé]

On 24/3/26 17:35, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Pylint complains here:
> 
>   config.py:1:0: C0114: Missing module docstring (missing-module-docstring)
>   config.py:28:4: W0719: Raising too general exception: Exception (broad-exception-raised)
> 
> Add a module description and replace the general Exception to fix this.
> And while we're at it, and since we've got a proper module description
> string now, also replace the copy-n-pasted comment at the top of the file
> with a proper SPDX identifier.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/functional/qemu_test/config.py | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>