Addon Manager: Linter cleanup of new test code

Author: chennes

src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py

@@ -21,14 +21,18 @@
 # *                                                                         *
 # ***************************************************************************
 
+""" Test the Change Branch GUI code"""
+
+# pylint: disable=wrong-import-position, deprecated-module, too-many-return-statements
+
 import sys
 import unittest
 from unittest.mock import patch, Mock, MagicMock
 
 # So that when run standalone, the Addon Manager classes imported below are available
 sys.path.append("../..")
 
-from AddonManagerTest.gui.gui_mocks import DialogWatcher, DialogInteractor, AsynchronousMonitor
+from AddonManagerTest.gui.gui_mocks import DialogWatcher, AsynchronousMonitor
 
 from change_branch import ChangeBranchDialog
 
@@ -45,11 +49,14 @@
 
 
 class MockFilter(QtCore.QSortFilterProxyModel):
+    """Replaces a filter with a non-filter that simply always returns whatever it's given"""
+
     def mapToSource(self, something):
         return something
 
 
 class MockChangeBranchDialogModel(QtCore.QAbstractTableModel):
+    """Replace a data-connected model with a static one for testing"""
 
     branches = [
         {"ref_name": "ref1", "upstream": "us1"},
@@ -64,40 +71,45 @@ def __init__(self, _: str, parent=None) -> None:
         super().__init__(parent)
 
     def rowCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Number of rows: should always return 3"""
         if parent.isValid():
             return 0
         return len(self.branches)
 
     def columnCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Number of columns (identical to non-mocked version)"""
         if parent.isValid():
             return 0
         return 3  # Local name, remote name, date
 
     def data(self, index: QtCore.QModelIndex, role: int = QtCore.Qt.DisplayRole):
+        """Mock returns static untranslated strings for DisplayRole, no tooltips at all, and
+        otherwise matches the non-mock version"""
         if not index.isValid():
             return None
         row = index.row()
         column = index.column()
         if role == QtCore.Qt.DisplayRole:
             if column == 2:
                 return "date"
-            elif column == 0:
+            if column == 0:
                 return "ref_name"
-            elif column == 1:
+            if column == 1:
                 return "upstream"
-            else:
-                return None
-        elif role == MockChangeBranchDialogModel.DataSortRole:
             return None
-        elif role == MockChangeBranchDialogModel.RefAccessRole:
+        if role == MockChangeBranchDialogModel.DataSortRole:
+            return None
+        if role == MockChangeBranchDialogModel.RefAccessRole:
             return self.branches[row]
+        return None
 
     def headerData(
         self,
         section: int,
         orientation: QtCore.Qt.Orientation,
         role: int = QtCore.Qt.DisplayRole,
     ):
+        """Mock returns untranslated strings for DisplayRole, and no tooltips at all"""
         if orientation == QtCore.Qt.Vertical:
             return None
         if role != QtCore.Qt.DisplayRole:
@@ -106,16 +118,18 @@ def headerData(
             return "Local"
         if section == 1:
             return "Remote tracking"
-        elif section == 2:
+        if section == 2:
             return "Last Updated"
-        else:
-            return None
+        return None
 
     def currentBranch(self) -> str:
+        """Mock returns a static string stored in the class: that string could be modified to
+        return something else by tests that require it."""
         return self.current_branch
 
 
 class TestChangeBranchGui(unittest.TestCase):
+    """Tests for the ChangeBranch GUI code"""
 
     MODULE = "test_change_branch"  # file name without extension
 
@@ -128,6 +142,7 @@ def tearDown(self):
     @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
     @patch("change_branch.initialize_git", new=Mock(return_value=None))
     def test_no_git(self):
+        """If git is not present, a dialog saying so is presented"""
         # Arrange
         gui = ChangeBranchDialog("/some/path")
         ref = {"ref_name": "foo/bar", "upstream": "us1"}
@@ -137,14 +152,15 @@ def test_no_git(self):
         )
 
         # Act
-        gui._change_branch("/foo/bar/baz", ref)
+        gui.change_branch("/foo/bar/baz", ref)
 
         # Assert
         self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
 
     @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
     @patch("change_branch.initialize_git")
     def test_git_failed(self, init_git: MagicMock):
+        """If git fails when attempting to change branches, a dialog saying so is presented"""
         # Arrange
         git_manager = MagicMock()
         git_manager.checkout = MagicMock()
@@ -158,24 +174,24 @@ def test_git_failed(self, init_git: MagicMock):
         )
 
         # Act
-        gui._change_branch("/foo/bar/baz", ref)
+        gui.change_branch("/foo/bar/baz", ref)
 
         # Assert
         self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
 
     @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
     @patch("change_branch.initialize_git", new=MagicMock)
     def test_branch_change_succeeded(self):
-        # If nothing gets thrown, then the process is assumed to have worked, and the appropriate
-        # signal is emitted.
+        """If nothing gets thrown, then the process is assumed to have worked, and the appropriate
+        signal is emitted."""
 
         # Arrange
         gui = ChangeBranchDialog("/some/path")
         ref = {"ref_name": "foo/bar", "upstream": "us1"}
         monitor = AsynchronousMonitor(gui.branch_changed)
 
         # Act
-        gui._change_branch("/foo/bar/baz", ref)
+        gui.change_branch("/foo/bar/baz", ref)
 
         # Assert
         monitor.wait_for_at_most(10)  # Should be effectively instantaneous
@@ -185,6 +201,8 @@ def test_branch_change_succeeded(self):
     @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
     @patch("change_branch.initialize_git", new=MagicMock)
     def test_warning_is_shown_when_dialog_is_accepted(self):
+        """If the dialog is accepted (e.g. a branch change is requested) then a warning dialog is
+        displayed, and gives the opportunity to cancel. If cancelled, no signal is emitted."""
         # Arrange
         gui = ChangeBranchDialog("/some/path")
         gui.ui.exec = MagicMock()
@@ -197,12 +215,14 @@ def test_warning_is_shown_when_dialog_is_accepted(self):
             translate("AddonsInstaller", "DANGER: Developer feature"),
             QtWidgets.QDialogButtonBox.Cancel,
         )
+        monitor = AsynchronousMonitor(gui.branch_changed)
 
         # Act
         gui.exec()
 
         # Assert
         self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
+        self.assertFalse(monitor.good())  # The watcher cancelled the op, so no signal is emitted
 
 
 if __name__ == "__main__":

src/Mod/AddonManager/addonmanager_utilities.py

@@ -24,6 +24,8 @@
 
 """ Utilities to work across different platforms, providers and python versions """
 
+# pylint: disable=deprecated-module, ungrouped-imports
+
 from datetime import datetime
 from typing import Optional, Any, List
 import os
@@ -52,6 +54,7 @@
 except ImportError:
 
     def get_python_exe():
+        """Use shutil.which to find python executable"""
         return shutil.which("python")
 
 
@@ -61,9 +64,17 @@ def get_python_exe():
     # loop running this is not possible, so fall back to requests (if available), or the native
     # Python urllib.request (if requests is not available).
     import NetworkManager  # Requires an event loop, so is only available with the GUI
+
+    requests = None
+    ssl = None
+    urllib = None
 else:
+    NetworkManager = None
     try:
         import requests
+
+        ssl = None
+        urllib = None
     except ImportError:
         requests = None
         import urllib.request
@@ -85,11 +96,14 @@ def get_python_exe():
         except ImportError:
 
             def loadUi(ui_file: str):
+                """If there are no available versions of QtUiTools, then raise an error if this
+                method is used."""
                 raise RuntimeError("Cannot use QUiLoader without PySide or FreeCAD")
 
     if has_loader:
 
         def loadUi(ui_file: str) -> QtWidgets.QWidget:
+            """Load a Qt UI from an on-disk file."""
             q_ui_file = QtCore.QFile(ui_file)
             q_ui_file.open(QtCore.QFile.OpenModeFlag.ReadOnly)
             loader = QUiLoader()
@@ -135,10 +149,13 @@ def symlink(source, link_name):
 
 
 def rmdir(path: str) -> bool:
+    """Remove a directory or symlink, even if it is read-only."""
     try:
         if os.path.islink(path):
             os.unlink(path)  # Remove symlink
         else:
+            # NOTE: the onerror argument was deprecated in Python 3.12, replaced by onexc -- replace
+            # when earlier versions are no longer supported.
             shutil.rmtree(path, onerror=remove_readonly)
     except (WindowsError, PermissionError, OSError):
         return False
@@ -213,7 +230,7 @@ def get_zip_url(repo):
 
 
 def recognized_git_location(repo) -> bool:
-    """Returns whether this repo is based at a known git repo location: works with github, gitlab,
+    """Returns whether this repo is based at a known git repo location: works with GitHub, gitlab,
     framagit, and salsa.debian.org"""
 
     parsed_url = urlparse(repo.url)
@@ -395,7 +412,7 @@ def is_float(element: Any) -> bool:
 
 
 def get_pip_target_directory():
-    # Get the default location to install new pip packages
+    """Get the default location to install new pip packages"""
     major, minor, _ = platform.python_version_tuple()
     vendor_path = os.path.join(
         fci.DataPaths().mod_dir, "..", "AdditionalPythonPackages", f"py{major}{minor}"
@@ -417,7 +434,12 @@ def blocking_get(url: str, method=None) -> bytes:
     succeeded, or an empty string if it failed, or returned no data. The method argument is
     provided mainly for testing purposes."""
     p = b""
-    if fci.FreeCADGui and method is None or method == "networkmanager":
+    if (
+        fci.FreeCADGui
+        and method is None
+        or method == "networkmanager"
+        and NetworkManager is not None
+    ):
         NetworkManager.InitializeNetworkManager()
         p = NetworkManager.AM_NETWORK_MANAGER.blocking_get(url, 10000)  # 10 second timeout
         if p:
@@ -462,13 +484,13 @@ def run_interruptable_subprocess(args, timeout_secs: int = 10) -> subprocess.Com
             # one second timeout allows interrupting the run once per second
             stdout, stderr = p.communicate(timeout=1)
             return_code = p.returncode
-        except subprocess.TimeoutExpired:
+        except subprocess.TimeoutExpired as timeout_exception:
             if (
                 hasattr(QtCore, "QThread")
                 and QtCore.QThread.currentThread().isInterruptionRequested()
             ):
                 p.kill()
-                raise ProcessInterrupted()
+                raise ProcessInterrupted() from timeout_exception
             if time.time() - start_time >= timeout_secs:  # The real timeout
                 p.kill()
                 stdout, stderr = p.communicate()
@@ -481,9 +503,10 @@ def run_interruptable_subprocess(args, timeout_secs: int = 10) -> subprocess.Com
 
 
 def process_date_string_to_python_datetime(date_string: str) -> datetime:
-    """For modern macros the expected date format is ISO 8601, YYYY-MM-DD. For older macros this standard was not always
-    used, and various orderings and separators were used. This function tries to match the majority of those older
-    macros. Commonly-used separators are periods, slashes, and dashes."""
+    """For modern macros the expected date format is ISO 8601, YYYY-MM-DD. For older macros this
+    standard was not always used, and various orderings and separators were used. This function
+    tries to match the majority of those older macros. Commonly-used separators are periods,
+    slashes, and dashes."""
 
     def raise_error(bad_string: str, root_cause: Exception = None):
         raise ValueError(
@@ -499,19 +522,19 @@ def raise_error(bad_string: str, root_cause: Exception = None):
         # The earliest possible year an addon can be created or edited is 2001:
         if split_result[0] > 2000:
             return datetime(split_result[0], split_result[1], split_result[2])
-        elif split_result[2] > 2000:
-            # Generally speaking it's not possible to distinguish between DD-MM and MM-DD, so try the first, and
-            # only if that fails try the second
+        if split_result[2] > 2000:
+            # Generally speaking it's not possible to distinguish between DD-MM and MM-DD, so try
+            # the first, and only if that fails try the second
             if split_result[1] <= 12:
                 return datetime(split_result[2], split_result[1], split_result[0])
             return datetime(split_result[2], split_result[0], split_result[1])
-        else:
-            raise ValueError(f"Invalid year in date string '{date_string}'")
+        raise ValueError(f"Invalid year in date string '{date_string}'")
     except ValueError as exception:
         raise_error(date_string, exception)
 
 
 def get_main_am_window():
+    """Find the Addon Manager's main window in the Qt widget hierarchy."""
     windows = QtWidgets.QApplication.topLevelWidgets()
     for widget in windows:
         if widget.objectName() == "AddonManager_Main_Window":
@@ -540,7 +563,7 @@ def create_pip_call(args: List[str]) -> List[str]:
     else:
         python_exe = get_python_exe()
         if not python_exe:
-            raise (RuntimeError("Could not locate Python executable on this system"))
+            raise RuntimeError("Could not locate Python executable on this system")
         call_args = [python_exe, "-m", "pip", "--disable-pip-version-check"]
         call_args.extend(args)
     return call_args

src/Mod/AddonManager/change_branch.py

@@ -120,9 +120,9 @@ def exec(self):
                 if result == QtWidgets.QMessageBox.Cancel:
                     return
 
-            self._change_branch(self.item_model.path, ref)
+            self.change_branch(self.item_model.path, ref)
 
-    def _change_branch(self, path: str, ref: Dict[str, str]) -> None:
+    def change_branch(self, path: str, ref: Dict[str, str]) -> None:
         """Change the git clone in `path` to git ref `ref`. Emits the branch_changed signal
         on success."""
         remote_name = ref["ref_name"]