diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3cded63..c4f6561 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/kernel_patches_daemon/branch_worker.py b/kernel_patches_daemon/branch_worker.py index 70ad5ca..edf2f6c 100644 --- a/kernel_patches_daemon/branch_worker.py +++ b/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 diff --git a/kernel_patches_daemon/github_sync.py b/kernel_patches_daemon/github_sync.py index 8df9be7..e87f830 100644 --- a/kernel_patches_daemon/github_sync.py +++ b/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"] diff --git a/pyproject.toml b/pyproject.toml index 9081d08..c0930d8 100644 --- a/pyproject.toml +++ b/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"] diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index 6209346..f4c7396 100644 --- a/tests/test_branch_worker.py +++ b/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}" + ) diff --git a/tests/test_github_connector.py b/tests/test_github_connector.py index 538b797..8927c5f 100644 --- a/tests/test_github_connector.py +++ b/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 ) diff --git a/tests/test_github_sync.py b/tests/test_github_sync.py index 8bd8882..9080c36 100644 --- a/tests/test_github_sync.py +++ b/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