Fixes in github_sync and other updates

.github/workflows/ci.yml

@@ -18,7 +18,7 @@ jobs:
       fail-fast: true
       matrix:
         os: [ubuntu-latest]
-        python-version: ["3.9", "3.10", "3.11"]
+        python-version: ["3.10", "3.11", "3.12"]
     steps:
       - uses: actions/checkout@v4
 

kernel_patches_daemon/branch_worker.py

@@ -416,7 +416,7 @@ def parse_pr_ref(ref: str) -> Dict[str, Any]:
         res["target"] = tmp[1]
 
     tmp = res["series"].split("/", maxsplit=1)
-    if len(tmp) >= 2:
+    if len(tmp) >= 2 and tmp[1].isdigit():
         res["series_id"] = int(tmp[1])
 
     return res

kernel_patches_daemon/github_sync.py

@@ -321,7 +321,10 @@ async def sync_patches(self) -> None:
                 if worker._is_relevant_pr(pr):
                     parsed_ref = parse_pr_ref(pr.head.ref)
                     # ignore unknown format branch/PRs.
-                    if parsed_pr_ref_ok(parsed_ref):
+                    if not parsed_pr_ref_ok(parsed_ref):
+                        logger.warning(
+                            f"Unexpected format of the branch name: {pr.head.ref}"
+                        )
                         continue
 
                     series_id = parsed_ref["series_id"]

pyproject.toml

@@ -22,7 +22,7 @@ aiohttp = "^3.8"
 aiohttp-retry = "^2.8"
 aioresponses = "^0.7"
 multidict = "^6.0"
-python = "^3.9"
+python = "^3.10"
 cachetools = "^5.3.0"
 gitpython = "^3.1"
 opentelemetry-api = "^1.18.0"
@@ -33,7 +33,7 @@ python-dateutil = "^2.8.2"
 # Explicitly include cryptography package
 # It's required for PyJWT which is dependency of PyGithub
 # https://pyjwt.readthedocs.io/en/stable/installation.html#cryptographic-dependencies-optional
-cryptography = "^43.0.1"
+cryptography = "^44.0.1"
 certifi = "*"
 
 [tool.poetry.group.dev.dependencies]
@@ -58,4 +58,4 @@ requires = ["poetry-core"]
 build-backend = "poetry.core.masonry.api"
 
 [tool.black]
-target-version = ["py38", "py39", "py310", "py311"]
+target-version = ["py310", "py311", "py312"]

tests/test_branch_worker.py

@@ -34,6 +34,7 @@
     email_in_submitter_allowlist,
     EmailBodyContext,
     furnish_ci_email_body,
+    parse_pr_ref,
     same_series_different_target,
     temporary_patch_file,
     UPSTREAM_REMOTE_NAME,
@@ -226,9 +227,10 @@ class TestCase:
             with self.subTest(msg=case.name):
                 self._bw.repo.get_pulls.reset_mock()
                 self._bw.repo.get_pulls.return_value = case.prs
-                with patch.object(BranchWorker, "add_pr") as ap, patch.object(
-                    BranchWorker, "_is_relevant_pr"
-                ) as rp:
+                with (
+                    patch.object(BranchWorker, "add_pr") as ap,
+                    patch.object(BranchWorker, "_is_relevant_pr") as rp,
+                ):
                     # set is_relevant return values
                     rp.side_effect = [pr.relevant for pr in case.prs]
 
@@ -327,9 +329,10 @@ class R:
             lr.remote.assert_called_with(UPSTREAM_REMOTE_NAME)
 
     def test_do_sync_reset_repo(self) -> None:
-        with patch.object(self._bw, "repo_local") as lr, patch(
-            "kernel_patches_daemon.branch_worker._reset_repo"
-        ) as rr:
+        with (
+            patch.object(self._bw, "repo_local") as lr,
+            patch("kernel_patches_daemon.branch_worker._reset_repo") as rr,
+        ):
             # Create a mock suitable to mock a git.RemoteReference
             remote_ref = "a/b/c/d"
             m = MagicMock()
@@ -425,9 +428,10 @@ class TestCase:
     def test_fetch_repo_path_doesnt_exist_full_sync(self) -> None:
         """When the repo does not exist yet, a full sync is performed."""
         fetch_params = ["somepath", "giturl", "branch"]
-        with patch.object(self._bw, "full_sync") as fr, patch(
-            "kernel_patches_daemon.branch_worker.os.path.exists"
-        ) as exists:
+        with (
+            patch.object(self._bw, "full_sync") as fr,
+            patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists,
+        ):
             # path does not exists
             exists.return_value = False
             self._bw.fetch_repo(*fetch_params)
@@ -436,9 +440,10 @@ def test_fetch_repo_path_doesnt_exist_full_sync(self) -> None:
     def test_fetch_repo_path_exists_no_full_sync(self) -> None:
         """If the repo already exist, we don't perform a full sync."""
         fetch_params = ["somepath", "giturl", "branch"]
-        with patch.object(self._bw, "full_sync") as fr, patch(
-            "kernel_patches_daemon.branch_worker.os.path.exists"
-        ) as exists:
+        with (
+            patch.object(self._bw, "full_sync") as fr,
+            patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists,
+        ):
             # path does exists
             exists.return_value = True
             self._bw.fetch_repo(*fetch_params)
@@ -447,9 +452,10 @@ def test_fetch_repo_path_exists_no_full_sync(self) -> None:
     def test_fetch_repo_path_exists_git_exception(self) -> None:
         """When the repo exists but we hit a git command exception, we fallback on full sync."""
         fetch_params = ["somepath", "giturl", "branch"]
-        with patch.object(self._bw, "full_sync") as fr, patch(
-            "kernel_patches_daemon.branch_worker.os.path.exists"
-        ) as exists:
+        with (
+            patch.object(self._bw, "full_sync") as fr,
+            patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists,
+        ):
             # path does exists
             exists.return_value = True
             self._git_repo_mock.init.return_value.git.fetch.side_effect = (
@@ -519,9 +525,11 @@ class TestCase:
             with self.subTest(msg=case.name):
                 self._bw.branches = case.branches
                 self._bw.all_prs = {p: {} for p in case.all_prs}
-                with patch.object(self._bw, "filter_closed_pr") as fcp, patch.object(
-                    self._bw, "delete_branch"
-                ) as db, freeze_time(not_expired_time):
+                with (
+                    patch.object(self._bw, "filter_closed_pr") as fcp,
+                    patch.object(self._bw, "delete_branch") as db,
+                    freeze_time(not_expired_time),
+                ):
                     fcp.side_effect = case.fcp_return_prs
                     self._bw.expire_branches()
                     # check fcp and db are called with proper counts
@@ -1400,3 +1408,109 @@ def test_email_submitter_not_in_allowlist_and_allowlist_disabled(self):
         )
         self.assertEqual(expected_cmd, cmd)
         self.assertEqual(expected_email, email)
+
+
+class TestParsePrRef(unittest.TestCase):
+    def test_parse_pr_ref_comprehensive(self):
+        test_cases = [
+            (
+                "series/123456=>main",
+                {"series": "series/123456", "series_id": 123456, "target": "main"},
+            ),
+            (
+                "patch/789=>bpf-next",
+                {"series": "patch/789", "series_id": 789, "target": "bpf-next"},
+            ),
+            ("series/42", {"series": "series/42", "series_id": 42}),
+            ("series/abc=>target", {"series": "series/abc", "target": "target"}),
+            (
+                "series/999=>feature/branch-name",
+                {
+                    "series": "series/999",
+                    "series_id": 999,
+                    "target": "feature/branch-name",
+                },
+            ),
+            ("", {"series": ""}),
+            ("=>", {"series": "", "target": ""}),
+            ("series", {"series": "series"}),
+            ("=>target", {"series": "", "target": "target"}),
+            (
+                "series/123=>target=>extra",
+                {"series": "series/123", "series_id": 123, "target": "target=>extra"},
+            ),
+            (
+                "path/to/series/456=>target",
+                {"series": "path/to/series/456", "target": "target"},
+            ),
+            ("series/abc123=>target", {"series": "series/abc123", "target": "target"}),
+            ("series/123abc=>target", {"series": "series/123abc", "target": "target"}),
+            (
+                " series/123=>target ",
+                {"series": " series/123", "series_id": 123, "target": "target "},
+            ),
+            (
+                "series/1=>target",
+                {"series": "series/1", "series_id": 1, "target": "target"},
+            ),
+            (
+                "series/007=>target",
+                {"series": "series/007", "series_id": 7, "target": "target"},
+            ),
+            (
+                "series/000=>target",
+                {"series": "series/000", "series_id": 0, "target": "target"},
+            ),
+            ("series/12a34=>target", {"series": "series/12a34", "target": "target"}),
+            ("series/a1234=>target", {"series": "series/a1234", "target": "target"}),
+            ("series/abc=>target", {"series": "series/abc", "target": "target"}),
+            ("series/=>target", {"series": "series/", "target": "target"}),
+            (
+                "path/123/series/456=>target",
+                {"series": "path/123/series/456", "target": "target"},
+            ),
+            ("series/123!@#=>target", {"series": "series/123!@#", "target": "target"}),
+            (
+                "série/123=>tärget",
+                {"series": "série/123", "series_id": 123, "target": "tärget"},
+            ),
+            (
+                "series/999999999999999999999=>target",
+                {
+                    "series": "series/999999999999999999999",
+                    "series_id": 999999999999999999999,
+                    "target": "target",
+                },
+            ),
+            ("series/-123=>target", {"series": "series/-123", "target": "target"}),
+            (
+                "series/0=>target",
+                {"series": "series/0", "series_id": 0, "target": "target"},
+            ),
+            ("series/123.45=>target", {"series": "series/123.45", "target": "target"}),
+            ("series/1e5=>target", {"series": "series/1e5", "target": "target"}),
+            ("\n\r\t", {"series": "\n\r\t"}),
+            ("/" * 100, {"series": "/" * 100}),
+        ]
+
+        def validate_parsed_pr_ref(ref):
+            self.assertIsInstance(ref, dict)
+            self.assertIn("series", ref)
+            self.assertIsInstance(ref["series"], str)
+            if "target" in ref:
+                self.assertIsInstance(ref["target"], str)
+            if "series_id" in ref:
+                self.assertIsInstance(ref["series_id"], int)
+            valid_keys = {"series", "target", "series_id"}
+            self.assertTrue(set(result.keys()).issubset(valid_keys))
+
+        for test_input, expected in test_cases:
+            with self.subTest(input=test_input):
+                try:
+                    result = parse_pr_ref(test_input)
+                    validate_parsed_pr_ref(result)
+                    self.assertEqual(result, expected)
+                except Exception as e:
+                    self.fail(
+                        f"parse_pr_ref raised {type(e).__name__} for input '{test_input}': {e}"
+                    )

tests/test_github_connector.py

@@ -254,11 +254,14 @@ def test_renew_expired_token(self) -> None:
             munch.munchify({"token": "token1", "expires_at": expired_at_date}),
             munch.munchify({"token": "token2", "expires_at": expired_at_next}),
         ]
-        with patch.object(
-            AppInstallationAuth,
-            "_get_installation_authorization",
-            side_effect=side_effect,
-        ) as p, freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime:
+        with (
+            patch.object(
+                AppInstallationAuth,
+                "_get_installation_authorization",
+                side_effect=side_effect,
+            ) as p,
+            freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime,
+        ):
             gc = get_default_gc_app_auth_client()
             # Force generating a first token
             # pyre-fixme[16]: `github.MainClass.Github` has no attribute `__requester`.
@@ -286,11 +289,14 @@ def test_donot_renew_non_expired_token(self) -> None:
             munch.munchify({"token": "token1", "expires_at": expired_at_date}),
             munch.munchify({"token": "token2", "expires_at": expired_at_next}),
         ]
-        with patch.object(
-            AppInstallationAuth,
-            "_get_installation_authorization",
-            side_effect=side_effect,
-        ) as p, freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime:
+        with (
+            patch.object(
+                AppInstallationAuth,
+                "_get_installation_authorization",
+                side_effect=side_effect,
+            ) as p,
+            freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime,
+        ):
             gc = get_default_gc_app_auth_client()
             # Force generating a first token
             # pyre-fixme[16]: `github.MainClass.Github` has no attribute `__requester`.
@@ -319,11 +325,14 @@ def test_repo_url(self) -> None:
             munch.munchify({"token": "token1", "expires_at": expired_at_date}),
             munch.munchify({"token": "token2", "expires_at": expired_at_next}),
         ]
-        with patch.object(
-            AppInstallationAuth,
-            "_get_installation_authorization",
-            side_effect=side_effect,
-        ) as p, freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime:
+        with (
+            patch.object(
+                AppInstallationAuth,
+                "_get_installation_authorization",
+                side_effect=side_effect,
+            ) as p,
+            freeze_time(DEFAULT_FREEZE_DATE) as frozen_datetime,
+        ):
             gc_app_auth = get_default_gc_app_auth_client()
             gc_oauth = get_default_gc_oauth_client()
             # Force generating a first token
@@ -366,11 +375,14 @@ def test_set_user_token_in_url_when_not_present(self) -> None:
             munch.munchify({"token": "token1", "expires_at": expired_at_date}),
         ]
 
-        with patch.object(
-            AppInstallationAuth,
-            "_get_installation_authorization",
-            side_effect=side_effect,
-        ) as p, freeze_time(DEFAULT_FREEZE_DATE):
+        with (
+            patch.object(
+                AppInstallationAuth,
+                "_get_installation_authorization",
+                side_effect=side_effect,
+            ) as p,
+            freeze_time(DEFAULT_FREEZE_DATE),
+        ):
             gc_app_auth = get_default_gc_app_auth_client(
                 repo_url=f"https://127.0.0.1/{TEST_ORG}/{TEST_REPO}"
             )
@@ -417,11 +429,14 @@ class TestCase:
                     munch.munchify({"token": "token1", "expires_at": expired_at_date}),
                 ]
 
-                with patch.object(
-                    AppInstallationAuth,
-                    "_get_installation_authorization",
-                    side_effect=side_effect,
-                ) as p, freeze_time(DEFAULT_FREEZE_DATE):
+                with (
+                    patch.object(
+                        AppInstallationAuth,
+                        "_get_installation_authorization",
+                        side_effect=side_effect,
+                    ) as p,
+                    freeze_time(DEFAULT_FREEZE_DATE),
+                ):
                     gc_app_auth = get_default_gc_app_auth_client(
                         repo_url=case.initial_url
                     )

tests/test_github_sync.py

@@ -7,6 +7,7 @@
 # pyre-unsafe
 
 import copy
+import os
 import unittest
 from dataclasses import dataclass
 from typing import Any, Dict, Optional
@@ -330,9 +331,11 @@ async def test_sync_patches_pr_summary_success(self, m: aioresponses) -> None:
         For patchwork mocking, it uses real response examples stored at tests/data/
         """
 
-        test_data = load_test_data(
-            "tests/data/test_github_sync.test_sync_patches_pr_summary_success"
+        data_path = os.path.join(
+            os.path.dirname(__file__),
+            "data/test_github_sync.test_sync_patches_pr_summary_success",
         )
+        test_data = load_test_data(data_path)
         init_pw_responses(m, test_data)
 
         # Set up mocks and test data