fix: compare updates via release tag (#745)

## ℹ️ Description - Link to the related issue(s): Issue #N/A - Describe the motivation and context for this change. Ensure update-check compares against release tags instead of moving branch tips and keep tests/translations in sync. ## 📋 Changes Summary - compare release commit via tag name first and fall back only when missing - update update-checker tests for commit-ish resolution and tag-based release data - refresh German translations for update-checker log strings ### ⚙️ Type of Change Select the type(s) of change(s) included in this pull request: - [x] 🐞 Bug fix (non-breaking change which fixes an issue) ## ✅ Checklist Before requesting a review, confirm the following: - [x] I have reviewed my changes to ensure they meet the project's standards. - [x] I have tested my changes and ensured that all tests pass (`pdm run test`). - [x] I have formatted the code (`pdm run format`). - [x] I have verified that linting passes (`pdm run lint`). - [x] I have updated documentation where necessary. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.  ## Summary by CodeRabbit * **Bug Fixes** * More reliable update checks by resolving commits from tags, branches or hashes and robustly comparing short vs full hashes. * Improved prerelease handling to avoid inappropriate preview updates and better handling of missing release data. * **Localization & UX** * Error and prerelease messages now use localized strings; commit dates shown consistently in UTC and short-hash form. * **Tests** * Updated tests to cover the new resolution flow, error cases, and logging behavior. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
2026-03-12 02:31:45 +01:00 · 2025-12-21 16:45:20 +01:00
parent 01753d0cba
commit 8ea41d3230
3 changed files with 274 additions and 150 deletions
--- a/tests/unit/test_update_checker.py
+++ b/tests/unit/test_update_checker.py
@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: © jens Bergmann and contributors
+# SPDX-FileCopyrightText: © Jens Bergmann and contributors
 # SPDX-License-Identifier: AGPL-3.0-or-later
 # SPDX-ArtifactOfProjectHomePage: https://github.com/Second-Hand-Friends/kleinanzeigen-bot/

@@ -7,7 +7,7 @@ from __future__ import annotations
 import json
 import logging
 from datetime import datetime, timedelta, timezone, tzinfo
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any, cast
 from unittest.mock import MagicMock, patch

 if TYPE_CHECKING:
@@ -90,29 +90,56 @@ class TestUpdateChecker:
        assert checker._get_commit_hash("2025+fb00f11") == "fb00f11"
        assert checker._get_commit_hash("2025") is None

-    def test_get_release_commit(self, config:Config) -> None:
-        """Test that the release commit hash is correctly retrieved from the GitHub API."""
+    def test_resolve_commitish(self, config:Config) -> None:
+        """Test that a commit-ish is resolved to a full hash and date."""
        checker = UpdateChecker(config)
-        with patch("requests.get", return_value = MagicMock(json = lambda: {"target_commitish": "e7a3d46"})):
-            assert checker._get_release_commit("latest") == "e7a3d46"
+        with patch(
+            "requests.get",
+            return_value = MagicMock(json = lambda: {"sha": "e7a3d46", "commit": {"author": {"date": "2025-05-18T00:00:00Z"}}})
+        ):
+            commit_hash, commit_date = checker._resolve_commitish("latest")
+            assert commit_hash == "e7a3d46"
+            assert commit_date == datetime(2025, 5, 18, tzinfo = timezone.utc)

    def test_request_timeout_uses_config(self, config:Config, mocker:"MockerFixture") -> None:
        """Ensure HTTP calls honor the timeout configuration."""
        config.timeouts.multiplier = 1.5
        checker = UpdateChecker(config)
-        mock_response = MagicMock(json = lambda: {"target_commitish": "abc"})
+        mock_response = MagicMock(json = lambda: {"sha": "abc", "commit": {"author": {"date": "2025-05-18T00:00:00Z"}}})
        mock_get = mocker.patch("requests.get", return_value = mock_response)

-        checker._get_release_commit("latest")
+        checker._resolve_commitish("latest")

        expected_timeout = config.timeouts.effective("update_check")
        assert mock_get.call_args.kwargs["timeout"] == expected_timeout

-    def test_get_commit_date(self, config:Config) -> None:
-        """Test that the commit date is correctly retrieved from the GitHub API."""
+    def test_resolve_commitish_no_commit(self, config:Config, mocker:"MockerFixture") -> None:
+        """Test resolving a commit-ish when the API returns no commit data."""
        checker = UpdateChecker(config)
-        with patch("requests.get", return_value = MagicMock(json = lambda: {"commit": {"author": {"date": "2025-05-18T00:00:00Z"}}})):
-            assert checker._get_commit_date("e7a3d46") == datetime(2025, 5, 18, tzinfo = timezone.utc)
+        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"sha": "abc"}))
+        commit_hash, commit_date = checker._resolve_commitish("sha")
+        assert commit_hash == "abc"
+        assert commit_date is None
+
+    def test_resolve_commitish_logs_warning_on_exception(
+        self,
+        config:Config,
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test resolving a commit-ish logs a warning when the request fails."""
+        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
+        checker = UpdateChecker(config)
+        with patch("requests.get", side_effect = Exception("boom")):
+            commit_hash, commit_date = checker._resolve_commitish("sha")
+
+        assert commit_hash is None
+        assert commit_date is None
+        assert any("Could not resolve commit 'sha': boom" in r.getMessage() for r in caplog.records)
+
+    def test_commits_match_short_hash(self, config:Config) -> None:
+        """Test that short commit hashes are treated as matching prefixes."""
+        checker = UpdateChecker(config)
+        assert checker._commits_match("abc1234", "abc1234def5678") is True

    def test_check_for_updates_disabled(self, config:Config) -> None:
        """Test that the update checker does not check for updates if disabled."""
@@ -125,9 +152,24 @@ class TestUpdateChecker:
    def test_check_for_updates_no_local_version(self, config:Config) -> None:
        """Test that the update checker handles the case where the local version cannot be determined."""
        checker = UpdateChecker(config)
-        with patch.object(UpdateChecker, "get_local_version", return_value = None):
+        with patch.object(UpdateCheckState, "should_check", return_value = True), \
+                patch.object(UpdateChecker, "get_local_version", return_value = None):
            checker.check_for_updates()  # Should not raise exception

+    def test_check_for_updates_logs_missing_local_version(
+        self,
+        config:Config,
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test that the update checker logs a warning when the local version is missing."""
+        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
+        checker = UpdateChecker(config)
+        with patch.object(UpdateCheckState, "should_check", return_value = True), \
+                patch.object(UpdateChecker, "get_local_version", return_value = None):
+            checker.check_for_updates()
+
+        assert any("Could not determine local version." in r.getMessage() for r in caplog.records)
+
    def test_check_for_updates_no_commit_hash(self, config:Config) -> None:
        """Test that the update checker handles the case where the commit hash cannot be extracted."""
        checker = UpdateChecker(config)
@@ -146,24 +188,48 @@ class TestUpdateChecker:
        with patch("requests.get", side_effect = Exception("API Error")):
            checker.check_for_updates()  # Should not raise exception

+    def test_check_for_updates_latest_prerelease_warning(
+        self,
+        config:Config,
+        mocker:"MockerFixture",
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test that the update checker warns when latest points to a prerelease."""
+        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
+        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
+        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
+        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
+        mocker.patch.object(
+            requests,
+            "get",
+            return_value = mocker.Mock(json = lambda: {"tag_name": "latest", "prerelease": True})
+        )
+
+        checker = UpdateChecker(config)
+        checker.check_for_updates()
+
+        expected = "Latest release from GitHub is a prerelease, but 'latest' channel expects a stable release."
+        assert any(expected in r.getMessage() for r in caplog.records)
+
    def test_check_for_updates_ahead(self, config:Config, mocker:"MockerFixture", caplog:pytest.LogCaptureFixture) -> None:
        """Test that the update checker correctly identifies when the local version is ahead of the latest release."""
        caplog.set_level("INFO", logger = "kleinanzeigen_bot.update_checker")
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "e7a3d46")
        mocker.patch.object(
            UpdateChecker,
-            "_get_commit_date",
+            "_resolve_commitish",
            side_effect = [
-                datetime(2025, 5, 18, tzinfo = timezone.utc),
-                datetime(2025, 5, 16, tzinfo = timezone.utc)
+                ("fb00f11", datetime(2025, 5, 18, tzinfo = timezone.utc)),
+                ("e7a3d46", datetime(2025, 5, 16, tzinfo = timezone.utc))
            ]
        )
        mocker.patch.object(
            requests,
            "get",
-            return_value = mocker.Mock(json = lambda: {"tag_name": "latest", "prerelease": False})
+            return_value = mocker.Mock(
+                json = lambda: {"tag_name": "latest", "prerelease": False}
+            )
        )
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)

@@ -186,19 +252,20 @@ class TestUpdateChecker:
        config.update_check.channel = "preview"
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "e7a3d46")
        mocker.patch.object(
            UpdateChecker,
-            "_get_commit_date",
+            "_resolve_commitish",
            side_effect = [
-                datetime(2025, 5, 18, tzinfo = timezone.utc),
-                datetime(2025, 5, 16, tzinfo = timezone.utc)
+                ("fb00f11", datetime(2025, 5, 18, tzinfo = timezone.utc)),
+                ("e7a3d46", datetime(2025, 5, 16, tzinfo = timezone.utc))
            ]
        )
        mocker.patch.object(
            requests,
            "get",
-            return_value = mocker.Mock(json = lambda: [{"tag_name": "preview", "prerelease": True}])
+            return_value = mocker.Mock(
+                json = lambda: [{"tag_name": "preview", "prerelease": True, "draft": False}]
+            )
        )
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)

@@ -216,24 +283,48 @@ class TestUpdateChecker:
        )
        assert any(expected in r.getMessage() for r in caplog.records)

+    def test_check_for_updates_preview_missing_prerelease(
+        self,
+        config:Config,
+        mocker:"MockerFixture",
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test that the update checker warns when no preview prerelease is available."""
+        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
+        config.update_check.channel = "preview"
+        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
+        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
+        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
+        mocker.patch.object(
+            requests,
+            "get",
+            return_value = mocker.Mock(json = lambda: [{"tag_name": "v1", "prerelease": False, "draft": False}])
+        )
+
+        checker = UpdateChecker(config)
+        checker.check_for_updates()
+
+        assert any("No prerelease found for 'preview' channel." in r.getMessage() for r in caplog.records)
+
    def test_check_for_updates_behind(self, config:Config, mocker:"MockerFixture", caplog:pytest.LogCaptureFixture) -> None:
        """Test that the update checker correctly identifies when the local version is behind the latest release."""
        caplog.set_level("INFO", logger = "kleinanzeigen_bot.update_checker")
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "e7a3d46")
        mocker.patch.object(
            UpdateChecker,
-            "_get_commit_date",
+            "_resolve_commitish",
            side_effect = [
-                datetime(2025, 5, 16, tzinfo = timezone.utc),
-                datetime(2025, 5, 18, tzinfo = timezone.utc)
+                ("fb00f11", datetime(2025, 5, 16, tzinfo = timezone.utc)),
+                ("e7a3d46", datetime(2025, 5, 18, tzinfo = timezone.utc))
            ]
        )
        mocker.patch.object(
            requests,
            "get",
-            return_value = mocker.Mock(json = lambda: {"tag_name": "latest", "prerelease": False})
+            return_value = mocker.Mock(
+                json = lambda: {"tag_name": "latest", "prerelease": False}
+            )
        )
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)

@@ -247,21 +338,57 @@ class TestUpdateChecker:
        expected = "A new version is available: e7a3d46 from 2025-05-18 00:00:00 UTC (current: 2025+fb00f11 from 2025-05-16 00:00:00 UTC, channel: latest)"
        assert any(expected in r.getMessage() for r in caplog.records)

+    def test_check_for_updates_logs_release_notes(
+        self,
+        config:Config,
+        mocker:"MockerFixture",
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test that release notes are logged when present."""
+        caplog.set_level("INFO", logger = "kleinanzeigen_bot.update_checker")
+        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
+        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
+        mocker.patch.object(
+            UpdateChecker,
+            "_resolve_commitish",
+            side_effect = [
+                ("fb00f11", datetime(2025, 5, 16, tzinfo = timezone.utc)),
+                ("e7a3d46", datetime(2025, 5, 18, tzinfo = timezone.utc))
+            ]
+        )
+        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
+        mocker.patch.object(
+            requests,
+            "get",
+            return_value = mocker.Mock(
+                json = lambda: {"tag_name": "latest", "prerelease": False, "body": "Release notes here"}
+            )
+        )
+
+        checker = UpdateChecker(config)
+        checker.check_for_updates()
+
+        assert any("Release notes:\nRelease notes here" in r.getMessage() for r in caplog.records)
+
    def test_check_for_updates_same(self, config:Config, mocker:"MockerFixture", caplog:pytest.LogCaptureFixture) -> None:
        """Test that the update checker correctly identifies when the local version is the same as the latest release."""
        caplog.set_level("INFO", logger = "kleinanzeigen_bot.update_checker")
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "fb00f11")
        mocker.patch.object(
            UpdateChecker,
-            "_get_commit_date",
-            return_value = datetime(2025, 5, 18, tzinfo = timezone.utc)
+            "_resolve_commitish",
+            side_effect = [
+                ("fb00f11", datetime(2025, 5, 18, tzinfo = timezone.utc)),
+                ("fb00f11", datetime(2025, 5, 18, tzinfo = timezone.utc))
+            ]
        )
        mocker.patch.object(
            requests,
            "get",
-            return_value = mocker.Mock(json = lambda: {"tag_name": "latest", "prerelease": False})
+            return_value = mocker.Mock(
+                json = lambda: {"tag_name": "latest", "prerelease": False}
+            )
        )
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)

@@ -275,6 +402,26 @@ class TestUpdateChecker:
        expected = "You are on the latest version: 2025+fb00f11 (compared to fb00f11 in channel latest)"
        assert any(expected in r.getMessage() for r in caplog.records)

+    def test_check_for_updates_unknown_channel(
+        self,
+        config:Config,
+        mocker:"MockerFixture",
+        caplog:pytest.LogCaptureFixture
+    ) -> None:
+        """Test that the update checker warns on unknown update channels."""
+        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
+        cast(Any, config.update_check).channel = "unknown"
+        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
+        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
+        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
+        mock_get = mocker.patch("requests.get")
+
+        checker = UpdateChecker(config)
+        checker.check_for_updates()
+
+        mock_get.assert_not_called()
+        assert any("Unknown update channel: unknown" in r.getMessage() for r in caplog.records)
+
    def test_check_for_updates_respects_interval_gate(
        self,
        config:Config,
@@ -457,59 +604,40 @@ class TestUpdateChecker:
        # Should not raise
        state.save(state_file)

-    def test_get_release_commit_no_sha(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_release_commit with API returning no sha key."""
+    def test_resolve_commitish_no_author(self, config:Config, mocker:"MockerFixture") -> None:
+        """Test resolving a commit-ish when the API returns no author key."""
        checker = UpdateChecker(config)
-        mocker.patch("requests.get", return_value = mocker.Mock(json = dict))
-        assert checker._get_release_commit("latest") is None
+        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"sha": "abc", "commit": {}}))
+        commit_hash, commit_date = checker._resolve_commitish("sha")
+        assert commit_hash == "abc"
+        assert commit_date is None

-    def test_get_release_commit_list_instead_of_dict(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_release_commit with API returning a list instead of dict."""
+    def test_resolve_commitish_no_date(self, config:Config, mocker:"MockerFixture") -> None:
+        """Test resolving a commit-ish when the API returns no date key."""
+        checker = UpdateChecker(config)
+        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"sha": "abc", "commit": {"author": {}}}))
+        commit_hash, commit_date = checker._resolve_commitish("sha")
+        assert commit_hash == "abc"
+        assert commit_date is None
+
+    def test_resolve_commitish_list_instead_of_dict(self, config:Config, mocker:"MockerFixture") -> None:
+        """Test resolving a commit-ish when the API returns a list instead of dict."""
        checker = UpdateChecker(config)
        mocker.patch("requests.get", return_value = mocker.Mock(json = list))
-        assert checker._get_release_commit("latest") is None
+        commit_hash, commit_date = checker._resolve_commitish("sha")
+        assert commit_hash is None
+        assert commit_date is None

-    def test_get_commit_date_no_commit(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_commit_date with API returning no commit key."""
-        checker = UpdateChecker(config)
-        mocker.patch("requests.get", return_value = mocker.Mock(json = dict))
-        assert checker._get_commit_date("sha") is None
-
-    def test_get_commit_date_no_author(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_commit_date with API returning no author key."""
-        checker = UpdateChecker(config)
-        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"commit": {}}))
-        assert checker._get_commit_date("sha") is None
-
-    def test_get_commit_date_no_date(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_commit_date with API returning no date key."""
-        checker = UpdateChecker(config)
-        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"commit": {"author": {}}}))
-        assert checker._get_commit_date("sha") is None
-
-    def test_get_commit_date_list_instead_of_dict(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test _get_commit_date with API returning a list instead of dict."""
-        checker = UpdateChecker(config)
-        mocker.patch("requests.get", return_value = mocker.Mock(json = list))
-        assert checker._get_commit_date("sha") is None
-
-    def test_check_for_updates_release_commit_exception(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test check_for_updates handles exception in _get_release_commit."""
+    def test_check_for_updates_missing_release_commitish(self, config:Config, mocker:"MockerFixture") -> None:
+        """Test check_for_updates handles missing release commit-ish."""
        checker = UpdateChecker(config)
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", side_effect = Exception("fail"))
-        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
-        checker.check_for_updates()  # Should not raise
-
-    def test_check_for_updates_commit_date_exception(self, config:Config, mocker:"MockerFixture") -> None:
-        """Test check_for_updates handles exception in _get_commit_date."""
-        checker = UpdateChecker(config)
-        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "e7a3d46")
-        mocker.patch.object(UpdateChecker, "_get_commit_date", side_effect = Exception("fail"))
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
+        mocker.patch(
+            "requests.get",
+            return_value = mocker.Mock(json = lambda: {"prerelease": False})
+        )
        checker.check_for_updates()  # Should not raise

    def test_check_for_updates_no_releases_empty(self, config:Config, mocker:"MockerFixture") -> None:
@@ -531,11 +659,15 @@ class TestUpdateChecker:
        caplog.set_level("WARNING", logger = "kleinanzeigen_bot.update_checker")
        mocker.patch.object(UpdateChecker, "get_local_version", return_value = "2025+fb00f11")
        mocker.patch.object(UpdateChecker, "_get_commit_hash", return_value = "fb00f11")
-        mocker.patch.object(UpdateChecker, "_get_release_commit", return_value = "e7a3d46")
-        mocker.patch.object(UpdateChecker, "_get_commit_date", return_value = None)
+        mocker.patch.object(UpdateChecker, "_resolve_commitish", return_value = (None, None))
        mocker.patch.object(UpdateCheckState, "should_check", return_value = True)
        # Patch requests.get to avoid any real HTTP requests
-        mocker.patch("requests.get", return_value = mocker.Mock(json = lambda: {"tag_name": "latest", "prerelease": False}))
+        mocker.patch(
+            "requests.get",
+            return_value = mocker.Mock(
+                json = lambda: {"tag_name": "latest", "prerelease": False}
+            )
+        )
        checker = UpdateChecker(config)
        checker.check_for_updates()
        assert any("Could not determine commit dates for comparison." in r.getMessage() for r in caplog.records)