fix: issue where user got wrong warnings when updating plugins (#1786)

src/ape_plugins/_cli.py

@@ -170,7 +170,7 @@ def install(cli_ctx, plugins, skip_confirmation, upgrade):
 
         else:
             cli_ctx.logger.warning(
-                f"'{plugin.name}' is already installed. " f"Did you mean to include '--upgrade'."
+                f"'{plugin.name}' is already installed. Did you mean to include '--upgrade'?"
             )
 
     if failures_occurred:

src/ape_plugins/utils.py

@@ -38,32 +38,39 @@ class PluginType(Enum):
     """
 
 
+def _check_pip_freeze() -> str:
+    result = subprocess.check_output([sys.executable, "-m", "pip", "freeze"])
+    return result.decode("utf8")
+
+
 class _PipFreeze:
     cache: Optional[Set[str]] = None
 
     def get_plugins(self, use_cache: bool = True) -> Set[str]:
         if use_cache and self.cache is not None:
             return self.cache
 
-        output = subprocess.check_output([sys.executable, "-m", "pip", "freeze"])
+        output = _check_pip_freeze()
         lines = [
             p
-            for p in output.decode().splitlines()
+            for p in output.splitlines()
             if p.startswith("ape-") or (p.startswith("-e") and "ape-" in p)
         ]
 
-        new_lines = []
+        # NOTE: Package IDs should look like "name==version"
+        #  if version is available.
+        package_ids = []
         for package in lines:
             if "-e" in package:
-                new_lines.append(package.split(".git")[0].split("/")[-1])
+                package_ids.append(package.split(".git")[0].split("/")[-1])
             elif "@" in package:
-                new_lines.append(package.split("@")[0].strip())
+                package_ids.append(package.split("@")[0].strip())
             elif "==" in package:
-                new_lines.append(package.split("==")[0].strip())
+                package_ids.append(package)
             else:
-                new_lines.append(package)
+                package_ids.append(package)
 
-        self.cache = {x for x in new_lines}
+        self.cache = set(x for x in package_ids)
         return self.cache
 
 
@@ -296,7 +303,7 @@ class ModifyPluginResultHandler:
     def __init__(self, plugin: PluginMetadata):
         self._plugin = plugin
 
-    def handle_install_result(self, result) -> bool:
+    def handle_install_result(self, result: int) -> bool:
         if not self._plugin.check_installed(use_cache=False):
             self._log_modify_failed("install")
             return False
@@ -318,21 +325,23 @@ def handle_upgrade_result(self, result, version_before: str) -> bool:
             self._log_errors_occurred("upgrading")
             return False
 
-        pip_freeze_version = self._plugin.pip_freeze_version
-        if version_before == pip_freeze_version or not pip_freeze_version:
-            if self._plugin.version:
-                logger.info(f"'{self._plugin.name}' already has version '{self._plugin.version}'.")
-            else:
-                logger.info(f"'{self._plugin.name}' already up to date.")
-
+        version_now = self._plugin.pip_freeze_version
+        if version_now is not None and version_before == version_now:
+            logger.info(f"'{self._plugin.name}' already has version '{version_now}'.")
             return True
-        else:
+
+        elif self._plugin.pip_freeze_version:
             logger.success(
                 f"Plugin '{self._plugin.name}' has been "
                 f"upgraded to version {self._plugin.pip_freeze_version}."
             )
             return True
 
+        else:
+            # The process was successful but there is still no pip freeze version.
+            # This may happen when installing things from GitHub.
+            return True
+
     def handle_uninstall_result(self, result) -> bool:
         if self._plugin.check_installed(use_cache=False):
             self._log_modify_failed("uninstall")

tests/functional/test_plugins.py

@@ -5,30 +5,53 @@
 
 from ape_plugins.utils import (
     ApePluginsRepr,
+    ModifyPluginResultHandler,
     PluginGroup,
     PluginMetadata,
     PluginMetadataList,
     PluginType,
+    _PipFreeze,
 )
 
 CORE_PLUGINS = ("run",)
 AVAILABLE_PLUGINS = ("available", "installed")
 INSTALLED_PLUGINS = ("installed", "thirdparty")
 THIRD_PARTY = ("thirdparty",)
-VERSION = "0.6.2"
+VERSION = "0.7.0"
+
+
+def get_pip_freeze_output(version: str):
+    return f"FOOFOO==1.1.1\n-e git+ssh://[email protected]/ApeWorX/ape-{INSTALLED_PLUGINS[0]}.git@aaaaaaaabbbbbb3343#egg=ape-{INSTALLED_PLUGINS[0]}\naiohttp==3.8.5\nape-{THIRD_PARTY[0]}=={version}\n"  # noqa: E501
+
+
+@pytest.fixture(autouse=True)
+def mock_pip_freeze(mocker):
+    def fn(version: str):
+        patch = mocker.patch("ape_plugins.utils._check_pip_freeze")
+        patch.return_value = get_pip_freeze_output(version)
+        return patch
+
+    return fn
 
 
 @pytest.fixture(autouse=True)
-def plugin_test_env(mocker):
+def plugin_test_env(mocker, mock_pip_freeze):
     root = "ape_plugins.utils"
 
     # Prevent calling out to GitHub
     gh_mock = mocker.patch(f"{root}._get_available_plugins")
     gh_mock.return_value = {f"ape_{x}" for x in AVAILABLE_PLUGINS}
 
+    # Used when testing PipFreeze object itself but also extra avoids
+    # actually calling out pip ever in tests.
+    mock_pip_freeze(VERSION)
+
     # Prevent requiring plugins to be installed.
     installed_mock = mocker.patch(f"{root}._pip_freeze_plugins")
-    installed_mock.return_value = {f"ape-{x}" for x in INSTALLED_PLUGINS}
+    installed_mock.return_value = {
+        f"ape-{INSTALLED_PLUGINS[0]}",
+        f"ape-{INSTALLED_PLUGINS[1]}=={VERSION}",
+    }
 
     # Prevent version lookups.
     version_mock = mocker.patch(f"{root}.get_package_version")
@@ -72,11 +95,11 @@ def test_names(self, name):
 
     def test_model_validator_when_version_included_with_name(self):
         # This allows parsing requirements files easier
-        metadata = PluginMetadata(name="ape-foo-bar==0.5.0")
+        metadata = PluginMetadata(name="ape-foo-bar==0.7.0")
         assert metadata.name == "foo-bar"
-        assert metadata.version == "==0.5.0"
+        assert metadata.version == "==0.7.0"
 
-    @pytest.mark.parametrize("version", ("0.5.0", "v0.5.0", "0.5.0a123"))
+    @pytest.mark.parametrize("version", ("0.7.0", "v0.7.0", "0.7.0a123"))
     def test_version(self, version):
         metadata = PluginMetadata(name="foo", version=version)
         assert metadata.version == version
@@ -87,19 +110,19 @@ def test_install_str_without_version(self):
         assert actual == "ape-foo-bar"
 
     def test_install_str_with_version(self):
-        metadata = PluginMetadata(name="foo-bar", version="0.5.0")
+        metadata = PluginMetadata(name="foo-bar", version="0.7.0")
         actual = metadata.install_str
-        assert actual == "ape-foo-bar==0.5.0"
+        assert actual == "ape-foo-bar==0.7.0"
 
     def test_install_str_with_complex_constraint(self):
-        metadata = PluginMetadata(name="foo", version=">=0.5.0,<0.6.0")
+        metadata = PluginMetadata(name="foo", version=">=0.7.0,<0.8.0")
         actual = metadata.install_str
-        assert actual == "ape-foo>=0.5.0,<0.6.0"
+        assert actual == "ape-foo>=0.7.0,<0.8.0"
 
     def test_install_str_with_complex_constraint_in_name(self):
-        metadata = PluginMetadata(name="foo>=0.5.0,<0.6.0")
+        metadata = PluginMetadata(name="foo>=0.7.0,<0.8.0")
         actual = metadata.install_str
-        assert actual == "ape-foo>=0.5.0,<0.6.0"
+        assert actual == "ape-foo>=0.7.0,<0.8.0"
 
     def test_install_str_when_using_git_remote(self):
         url = "git+https://example.com/ape-foo/branch"
@@ -185,3 +208,39 @@ def test_repr_when_exception(self, mocker):
         group = PluginGroup(plugin_type=PluginType.INSTALLED)
 
         assert repr(group) == "<PluginGroup>"
+
+
+def test_pip_freeze_includes_version_when_available():
+    pip_freeze = _PipFreeze()
+    actual = pip_freeze.get_plugins()
+    expected = {f"ape-{INSTALLED_PLUGINS[0]}", f"ape-{THIRD_PARTY[0]}==0.7.0"}
+    assert actual == expected
+
+
+def test_handle_upgrade_result_when_upgrading_to_same_version(caplog, logger):
+    # NOTE: pip freeze mock also returns version 0.7.0 (so upgrade to same).
+    logger.set_level("INFO")  # Required for test.
+    plugin = PluginMetadata(name=THIRD_PARTY[0])
+    handler = ModifyPluginResultHandler(plugin)
+    handler.handle_upgrade_result(0, "0.7.0")
+    if records := caplog.records:
+        assert f"'{THIRD_PARTY[0]}' already has version '0.7.0'" in records[-1].message
+    else:
+        version_at_end = plugin.pip_freeze_version
+        pytest.fail(
+            "Missing logs when upgrading to same version 0.7.0. "
+            f"pip_freeze_version={version_at_end}"
+        )
+
+
+def test_handle_upgrade_result_when_no_pip_freeze_version_does_not_log(caplog):
+    plugin_no_version = INSTALLED_PLUGINS[0]  # Version not in pip-freeze
+    plugin = PluginMetadata(name=plugin_no_version)
+    handler = ModifyPluginResultHandler(plugin)
+    handler.handle_upgrade_result(0, "0.7.0")
+
+    log_parts = ("already has version", "already up to date")
+    messages = [x.message for x in caplog.records]
+    for message in messages:
+        for pt in log_parts:
+            assert pt not in message