diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d4e3e8c..7d6f265 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -67,10 +67,10 @@ repos: rev: aab412d509121cb5f7533134b7e67f9fab59c682 # frozen: v0.16.4 hooks: - id: ruff - files: ^(src|scripts|tests)/.+\.py$ + files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$ args: [--fix, --exit-non-zero-on-fix] - id: ruff-format - files: ^(src|scripts|tests)/.+\.py$ + files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$ - repo: https://github.com/pre-commit/mirrors-mypy rev: 7ff8d35ae36a7d2b968f2f90b4c723e292e594ee # frozen: v2.3.1 @@ -123,7 +123,9 @@ repos: rev: 3c94d276aa3005fa1f5a977946c9dc106135c083 # frozen: 1.39.10 hooks: - id: basedpyright - files: ^src/.+\.py$ + # Every Python path in the repository. Keep in sync with the + # `include` list under [tool.pyright] in pyproject.toml. + files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$ - repo: https://github.com/lfreleng-actions/gha-workflow-linter rev: 42aa4db931ab4c9da0930ae5ca2259ab08da79cb # frozen: v1.5.2 diff --git a/pyproject.toml b/pyproject.toml index df74537..dc85d59 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,6 +107,12 @@ dev = [ "coverage[toml]>=7.10.6", "responses>=0.25.8", + # Several tests read CliRunner's result.stderr, which click only + # captures separately from 8.2 onward; before that it raises + # ValueError. This is a test-only requirement, so the runtime floor + # above stays where it is: the package itself still works on 8.1.7. + "click>=8.2", + # Lint/format "ruff>=0.6.3", "mypy>=1.17.1", @@ -254,7 +260,10 @@ markers = [ [tool.pyright] pythonVersion = "3.11" -include = ["src"] +# Every Python path in the repository, so an editor's language server and +# the pre-commit hook agree on the checked set. Keep in sync with the +# basedpyright hook's `files` pattern in .pre-commit-config.yaml. +include = ["src", "tests", "scripts", "sitecustomize.py"] typeCheckingMode = "strict" reportMissingImports = "none" reportMissingTypeStubs = "none" @@ -278,6 +287,21 @@ reportIncompatibleVariableOverride = "error" reportOverlappingOverload = "error" reportUnsupportedDunderAll = "error" +# Test code is held to the same correctness rules as src/, but not to the +# same annotation rules. This mirrors the mypy stance above, where tests +# set disallow_untyped_defs = false. +[[tool.pyright.executionEnvironments]] +root = "tests" +# pytest resolves fixtures by parameter name at collection time, so +# annotating every `monkeypatch`, `tmp_path` and `mock_*` parameter adds +# no checking that the collection step does not already perform. +reportMissingParameterType = "none" +# @pytest.fixture and @pytest.mark.* read as untyped whenever pytest is +# not importable. The pre-commit hook runs basedpyright without a project +# virtualenv, so this fires there but not locally. The same tolerance is +# already applied repository-wide via reportMissingImports = "none". +reportUntypedFunctionDecorator = "none" + [tool.uv] # uv will manage installation based on this pyproject and lockfile. @@ -289,4 +313,11 @@ managed = true dev = [ "hatch-vcs>=0.5.0", "hatchling>=1.28.0", + # Pull in the dev extra defined above. uv installs the `dev` group by + # default, so a bare `uv sync` provisions pytest and friends; without + # this, `uv run pytest` silently falls through to whatever pytest sits + # on PATH. Declared by reference rather than duplicated because CI + # installs the extra directly (`uv pip install ./[test,dev]`) and + # cannot see dependency groups. + "github2gerrit[dev]", ] diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index f8463c6..5648bc5 100644 --- a/src/github2gerrit/cli.py +++ b/src/github2gerrit/cli.py @@ -762,10 +762,18 @@ def _save_derived_parameters_after_success(data: Inputs) -> None: cfg = load_org_config(org_for_cfg) # Apply derivation with saving enabled to capture newly derived - # parameters + # parameters. The result is written to the configuration file + # only: this runs after submission and never calls + # apply_config_to_env, so nothing derived here reaches the + # process environment and recording it as derived provenance + # would describe environment values this call did not set. repository = os.getenv("GITHUB_REPOSITORY", "") apply_parameter_derivation( - cfg, org_for_cfg, repository=repository, save_to_config=True + cfg, + org_for_cfg, + repository=repository, + save_to_config=True, + mark_derived=False, ) log.debug( diff --git a/src/github2gerrit/config.py b/src/github2gerrit/config.py index 727cde9..d40f569 100644 --- a/src/github2gerrit/config.py +++ b/src/github2gerrit/config.py @@ -49,6 +49,7 @@ import logging import os import re +from collections.abc import Iterable from pathlib import Path from typing import Any from typing import cast @@ -476,6 +477,59 @@ def apply_config_to_env(cfg: dict[str, str]) -> None: os.environ[k] = v +# Provenance for configuration values. +# +# Several Gerrit parameters are *derived* when nothing configures them: +# the host from a `gerrit.[org].org` heuristic, the project from the +# GitHub repository name. Once `apply_config_to_env` exports them they +# are indistinguishable from values an operator set deliberately, so a +# consumer with a better answer of its own (duplicate detection reads +# the pull request's `.gitreview`) cannot tell whether overriding the +# environment would be correcting a guess or discarding intent. +# +# Recording the derived keys answers that question. The record lives in +# an environment variable because it describes *process-scoped +# configuration*, established once in `_load_effective_inputs` before +# the bulk `ThreadPoolExecutor` starts and identical for every pull +# request the process handles. That is the opposite of per-pull-request +# state such as the approved SHA, which `cli.py` deliberately threads +# through return values precisely because concurrent workers would +# otherwise overwrite one another's values. +DERIVED_KEYS_ENV = "G2G_DERIVED_KEYS" + + +def _derived_keys() -> set[str]: + """Return the set of config keys currently recorded as derived.""" + raw = os.getenv(DERIVED_KEYS_ENV, "") + return {part.strip().upper() for part in raw.split(",") if part.strip()} + + +def mark_derived_keys(keys: Iterable[str]) -> None: + """Record which config keys hold derived fallbacks rather than + operator intent. + + Additive: keys already recorded stay recorded, so several derivation + passes accumulate rather than the last one winning. + + Args: + keys: Config key names (case-insensitive). Empty names are + ignored. + """ + incoming = {k.strip().upper() for k in keys if k and k.strip()} + if not incoming: + return + os.environ[DERIVED_KEYS_ENV] = ",".join(sorted(_derived_keys() | incoming)) + + +def is_derived_key(key: str) -> bool: + """Return ``True`` when ``key``'s current value came from derivation. + + A ``False`` answer covers both "explicitly configured" and "not set + at all", so callers should test the value's presence separately. + """ + return key.strip().upper() in _derived_keys() + + def filter_known( cfg: dict[str, str], include_extra: bool = True, @@ -666,6 +720,8 @@ def apply_parameter_derivation( organization: str | None = None, repository: str | None = None, save_to_config: bool = True, + *, + mark_derived: bool = True, ) -> dict[str, str]: """Apply dynamic parameter derivation for missing Gerrit parameters. @@ -689,6 +745,11 @@ def apply_parameter_derivation( organization: GitHub organization name for derivation repository: GitHub repository in owner/repo format (optional) save_to_config: Whether to save derived parameters to config file + mark_derived: Whether to record the derived keys as provenance + (see :func:`mark_derived_keys`). Pass ``False`` from call + sites that do not follow up with :func:`apply_config_to_env`, + since a key that never reaches the environment must not be + described as holding a derived value there. Returns: Configuration dictionary with derived values for missing parameters @@ -737,6 +798,34 @@ def apply_parameter_derivation( "GitHub Actions" if is_github_actions else "Local CLI", ", ".join(f"{k}={v}" for k, v in newly_derived.items()), ) + if mark_derived: + # `newly_derived` records what derivation *supplied*, not + # what will end up in the environment. `apply_config_to_env` + # writes a key only when its environment value is currently + # empty, so a key derived here can still be shadowed by an + # explicit environment value and must not be recorded as + # derived. Apply the same emptiness test the caller will + # apply moments later; the two must agree. + mark_derived_keys( + key + for key in newly_derived + if (os.getenv(key) or "").strip() == "" + ) + + # A GERRIT_PROJECT arriving from the configuration file is a + # fallback, never intent for this specific repository: the file is + # sectioned per-organization, so one project name there is wrong for + # every repository in the org but one. That covers entries auto-saved + # by releases before such writes stopped, and hand-written ones, + # which are equally misscoped. Marking it derived keeps it usable as + # a last resort while letting the per-pull-request .gitreview + # outrank it. + if ( + mark_derived + and cfg.get("GERRIT_PROJECT", "").strip() + and (os.getenv("GERRIT_PROJECT") or "").strip() == "" + ): + mark_derived_keys(["GERRIT_PROJECT"]) # Save newly derived parameters to configuration file for future use # Default to true for local CLI, false for GitHub Actions default_auto_save = "false" if _is_github_actions_context() else "true" @@ -789,6 +878,18 @@ def save_derived_parameters_to_config( if not organization or not derived_params: return + # GERRIT_PROJECT is per-repository, but this file section is + # per-organization: persisting it would hand every other repository + # in the org one repository's project name, and would additionally + # come back on the next run as configuration indistinguishable from + # operator intent, defeating the provenance tracking that lets + # duplicate detection prefer a per-pull-request .gitreview. + derived_params = { + k: v for k, v in derived_params.items() if k != "GERRIT_PROJECT" + } + if not derived_params: + return + if config_path is None: config_path = ( os.getenv("G2G_CONFIG_PATH", "").strip() or DEFAULT_CONFIG_PATH diff --git a/src/github2gerrit/duplicate_detection.py b/src/github2gerrit/duplicate_detection.py index 40b932a..860fa85 100644 --- a/src/github2gerrit/duplicate_detection.py +++ b/src/github2gerrit/duplicate_detection.py @@ -223,16 +223,24 @@ def _resolve_gerrit_info_from_env_or_gitreview( :mod:`github2gerrit.gitreview` module which consolidates parsing, URL-encoding, branch deduplication, and URL validation. + Explicit configuration outranks the per-pull-request + ``.gitreview`` field by field, since derivation guesses + ``GERRIT_PROJECT`` from the repository name. A derived pair is + the last resort: a possibly-wrong project beats no query. + Returns: Tuple of (host, project) if found, None otherwise """ + from .config import is_derived_key from .gitreview import fetch_gitreview_raw - # First try environment variables (same as core module) gerrit_host = os.getenv("GERRIT_SERVER", "").strip() gerrit_project = os.getenv("GERRIT_PROJECT", "").strip() - if gerrit_host and gerrit_project: + env_ok = bool(gerrit_host and gerrit_project) + host_ok = bool(gerrit_host) and not is_derived_key("GERRIT_SERVER") + proj_ok = bool(gerrit_project) and not is_derived_key("GERRIT_PROJECT") + if host_ok and proj_ok: return (gerrit_host, gerrit_project) # Skip local .gitreview check in composite action context. @@ -243,7 +251,7 @@ def _resolve_gerrit_info_from_env_or_gitreview( repo_full = gh.repository.strip() if gh.repository else "" if not repo_full: - return None + return (gerrit_host, gerrit_project) if env_ok else None # Collect branch hints from the GitHub context. A fork may name # its branch after one that exists in the base repository, so an @@ -269,9 +277,13 @@ def _resolve_gerrit_info_from_env_or_gitreview( default_branches=default_branches, ) if info: - return (info.host, info.project) + # .gitreview fills non-explicit fields, where it has one. + host = gerrit_host if host_ok else info.host or gerrit_host + proj = gerrit_project if proj_ok else info.project or gerrit_project + if host and proj: + return (host, proj) - return None + return (gerrit_host, gerrit_project) if env_ok else None def _build_gerrit_rest_client(self, gerrit_host: str) -> Any | None: """Build a Gerrit REST API client using centralized framework. diff --git a/tests/conftest.py b/tests/conftest.py index efdc1a4..5699dc3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -231,6 +231,74 @@ def reset_gerrit_base_path_cache() -> Iterable[None]: gerrit_urls._BASE_PATH_CACHE.clear() +@pytest.fixture(autouse=True) +def reset_credential_caches() -> Iterable[None]: + """Clear the process-wide credential caches around every test. + + ⚠️ IMPORTANT: autouse=True is REQUIRED for test suite stability + + ``ssh_config_parser`` memoises four things for the lifetime of the + process, and every one of them derives its value from state that + tests routinely monkeypatch: + + * ``_get_respect_user_ssh_setting`` — ``lru_cache(maxsize=1)`` over a + zero-argument function reading ``G2G_RESPECT_USER_SSH``. With no + arguments there is a single cache slot, so *any* two tests collide + on it and the second receives the first one's environment. + * ``_get_cached_git_user_email`` — ``lru_cache(maxsize=1)`` over a + zero-argument function that shells out to ``git config``. Same + single-slot collision, over the git identity that + ``isolate_git_environment`` takes care to control. + * ``derive_gerrit_credentials`` — ``lru_cache(maxsize=32)`` keyed on + ``(host, organization, port)``. Tests share fixture hosts and + organisations, so the key collides while the answer depends on + the two caches above plus the SSH config on disk. + * ``_ssh_config_cache`` — keyed on ``~/.ssh/config`` as a path + string, never on that file's contents, so rewriting the file + under an unchanged ``HOME`` serves the previous parse. + + ``clear_credential_cache()`` resets all four. Two test modules call + it from ``setup_method``, which protects them on the way in but + leaves the caches populated for whatever runs next. Doing it here + instead covers every test, and clearing on both sides stops a dirty + cache reaching a test as well as leaving one behind. + """ + from github2gerrit.ssh_config_parser import clear_credential_cache + + clear_credential_cache() + yield + clear_credential_cache() + + +@pytest.fixture(autouse=True) +def reset_derived_key_provenance() -> Iterable[None]: + """Clear the derived-config-key record around every test. + + ⚠️ IMPORTANT: autouse=True is REQUIRED for test suite stability + + ``config.mark_derived_keys`` records provenance in the + ``G2G_DERIVED_KEYS`` environment variable, and is deliberately + additive so repeated derivation passes accumulate. Any test that + calls ``apply_parameter_derivation`` therefore leaves a record + behind, and it outlives ``monkeypatch`` teardown whenever the test + reached ``os.environ`` directly rather than through the fixture. + + A stale record makes ``config.is_derived_key`` answer ``True`` for a + key a later test set explicitly, which is exactly the distinction + duplicate detection relies on -- so the leak would surface as a + short-circuit that silently stopped happening, in an unrelated + module, depending on test order. + + Clearing on both sides keeps a dirty record from reaching a test and + stops one leaving a record behind. + """ + from github2gerrit.config import DERIVED_KEYS_ENV + + os.environ.pop(DERIVED_KEYS_ENV, None) + yield + os.environ.pop(DERIVED_KEYS_ENV, None) + + @pytest.fixture(autouse=True) def isolate_git_environment(monkeypatch: pytest.MonkeyPatch) -> None: """ diff --git a/tests/fixtures/make_repo.py b/tests/fixtures/make_repo.py index f45724a..a27d553 100644 --- a/tests/fixtures/make_repo.py +++ b/tests/fixtures/make_repo.py @@ -87,16 +87,9 @@ def _run_git( ) if check and proc.returncode != 0: cmd_str = " ".join(full_cmd) - stderr = ( - proc.stderr.decode("utf-8", errors="replace") - if proc.stderr is not None and isinstance(proc.stderr, bytes) - else proc.stderr - ) - stdout = ( - proc.stdout.decode("utf-8", errors="replace") - if proc.stdout is not None and isinstance(proc.stdout, bytes) - else proc.stdout - ) + # capture_output=True with text=False always yields bytes. + stderr = proc.stderr.decode("utf-8", errors="replace") + stdout = proc.stdout.decode("utf-8", errors="replace") raise RuntimeError( f"Git command failed: {cmd_str}\n" f"Return code: {proc.returncode}\n" diff --git a/tests/test_action_environment_mapping.py b/tests/test_action_environment_mapping.py index a8bafa5..e106ffd 100644 --- a/tests/test_action_environment_mapping.py +++ b/tests/test_action_environment_mapping.py @@ -81,6 +81,9 @@ def test_github_context_mapping(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Test GitHub context mappings @@ -113,6 +116,9 @@ def test_computed_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # These are computed by the upstream "Extract PR number" step and @@ -135,6 +141,9 @@ def test_issue_id_conditional_mapping(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) issue_id_env = env_mapping.get("ISSUE_ID", "") @@ -149,6 +158,9 @@ def test_test_mode_environment(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # G2G_TEST_MODE should be explicitly set to false in production @@ -296,6 +308,9 @@ def test_required_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Essential variables that must be present @@ -320,6 +335,9 @@ def test_boolean_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Boolean input variables @@ -348,6 +366,9 @@ def test_optional_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Optional variables that may be empty @@ -442,6 +463,9 @@ def test_secret_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Secret variables that should reference inputs or context @@ -463,6 +487,9 @@ def test_no_hardcoded_secrets(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Check that no environment values contain hardcoded secrets diff --git a/tests/test_action_outputs.py b/tests/test_action_outputs.py index 38bcecd..7c34745 100644 --- a/tests/test_action_outputs.py +++ b/tests/test_action_outputs.py @@ -119,6 +119,9 @@ def test_capture_step_script(self, action_config): None, ) + assert capture_step is not None, ( + "No step whose name contains 'Capture outputs' in action.yaml" + ) script = capture_step["run"] # Should use GitHub Actions multiline output format diff --git a/tests/test_cache_isolation.py b/tests/test_cache_isolation.py new file mode 100644 index 0000000..2900108 --- /dev/null +++ b/tests/test_cache_isolation.py @@ -0,0 +1,250 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation + +"""Regression tests for process-wide cache isolation between tests. + +Several modules memoise expensive lookups in module-level state that +outlives an individual test: ``gerrit_urls._BASE_PATH_CACHE`` and the +four caches in ``ssh_config_parser``. ``tests/conftest.py`` clears all +of them around every test via the ``reset_gerrit_base_path_cache`` and +``reset_credential_caches`` autouse fixtures. + +Each pair below is a polluter followed by a victim. The polluter +populates a cache with a value the victim must not see; the victim then +asserts the answer it would get from a clean process. Tests within a +module run in definition order, so the pairing is deterministic. Remove +either autouse fixture and the corresponding victim fails. +""" + +from __future__ import annotations + +from collections.abc import Callable +from pathlib import Path +from typing import Any + +import pytest + +from github2gerrit import gerrit_urls as urls_mod +from github2gerrit import ssh_config_parser as ssh_mod +from github2gerrit.gerrit_urls import create_gerrit_url_builder +from github2gerrit.ssh_config_parser import SSHConfig + + +# The fixture host shared by roughly twenty test modules, and therefore +# the one most likely to carry a leaked base path. +_SHARED_HOST = "gerrit.example.org" + + +class _FakeResp: + def __init__(self, code: int, headers: dict[str, str]) -> None: + self.status = code + self.headers = headers + + def getcode(self) -> int: + return self.status + + +class _FakeOpener: + def __init__( + self, decide: Callable[[str], _FakeResp | BaseException] + ) -> None: + self._decide = decide + self.addheaders: list[tuple[str, str]] = [] + + def open(self, url: str, timeout: float | None = None) -> _FakeResp: + result = self._decide(url) + if isinstance(result, BaseException): + raise result + return result + + +def _install_opener( + monkeypatch: pytest.MonkeyPatch, + decide: Callable[[str], _FakeResp | BaseException], +) -> None: + opener = _FakeOpener(decide) + + def _build(*_args: Any, **_kwargs: Any) -> _FakeOpener: + return opener + + monkeypatch.setattr( + "github2gerrit.gerrit_urls.urllib.request.build_opener", + _build, + raising=True, + ) + + +def test_base_path_cache_polluter_records_r_for_shared_host( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Populate the base-path cache with 'r' for the shared host. + + This mirrors what the base-path discovery tests do: a 302 whose + Location carries a ``/r/`` prefix teaches the module that the host + serves Gerrit under that base path, for the rest of the process. + """ + monkeypatch.delenv("GERRIT_HTTP_BASE_PATH", raising=False) + + def decide(url: str) -> _FakeResp | BaseException: + if url.endswith("/dashboard/self"): + return _FakeResp(302, {"Location": "/r/dashboard/self"}) + return _FakeResp(404, {}) + + _install_opener(monkeypatch, decide) + + builder = create_gerrit_url_builder(_SHARED_HOST) + + assert builder.base_path == "r" + assert urls_mod._BASE_PATH_CACHE[_SHARED_HOST] == "r" + + +def test_base_path_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A later, independent build for the same host sees no base path. + + Discovery here cannot reach the server, so the honest answer is an + empty base path. Without the autouse reset the builder short + circuits on the leaked ``'r'`` from the previous test and never + probes at all, yielding ``/r/`` URLs. + """ + monkeypatch.delenv("GERRIT_HTTP_BASE_PATH", raising=False) + + probed: list[str] = [] + + def decide(url: str) -> _FakeResp | BaseException: + probed.append(url) + return OSError("unreachable") + + _install_opener(monkeypatch, decide) + + builder = create_gerrit_url_builder(_SHARED_HOST) + + assert probed, "discovery short-circuited on a leaked cache entry" + assert builder.base_path == "" + assert ( + builder.change_url("releng/builder", 12345) + == "https://gerrit.example.org/c/releng/builder/+/12345" + ) + + +def test_credential_cache_polluter_records_ssh_derived_identity( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Populate the credential caches from a user-SSH-respecting run.""" + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + monkeypatch.setattr( + ssh_mod, + "get_ssh_user_for_gerrit", + lambda _host, _port=29418: "sshuser", + raising=True, + ) + monkeypatch.setattr( + ssh_mod, + "get_git_user_email", + lambda: "dev@example.org", + raising=True, + ) + + user, email = ssh_mod.derive_gerrit_credentials(_SHARED_HOST, "leakorg") + + assert user == "sshuser" + assert email == "dev@example.org" + assert ssh_mod._get_respect_user_ssh_setting() is True + + +def test_respect_user_ssh_setting_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The env-var cache reflects this test's environment, not the last. + + ``_get_respect_user_ssh_setting`` takes no arguments, so its + ``lru_cache(maxsize=1)`` has exactly one slot that every test in the + suite collides on, over an environment variable tests monkeypatch + freely. + """ + monkeypatch.delenv("G2G_RESPECT_USER_SSH", raising=False) + + assert ssh_mod._get_respect_user_ssh_setting() is False + + +def test_git_user_email_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The git-email cache reflects this test's git identity. + + ``_get_cached_git_user_email`` is the same shape as + ``_get_respect_user_ssh_setting``: zero arguments, one cache slot, + shared by the whole suite. What it memoises is the output of + ``git config user.email``, which ``isolate_git_environment`` and + individual tests both take pains to control. + """ + monkeypatch.setattr( + ssh_mod, + "get_git_user_email", + lambda: "victim@example.org", + raising=True, + ) + + assert ssh_mod._get_cached_git_user_email() == "victim@example.org" + + +def test_credential_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The same host and organisation derive fallback credentials. + + ``derive_gerrit_credentials`` is keyed only on host, organisation + and port. The polluter above used the same key, so without the + reset its answer is replayed here even though it was computed from + an environment and an SSH config this test does not share. + """ + monkeypatch.delenv("G2G_RESPECT_USER_SSH", raising=False) + + user, email = ssh_mod.derive_gerrit_credentials(_SHARED_HOST, "leakorg") + + assert user == "leakorg.gh2gerrit" + assert email == "releng+leakorg-gh2gerrit@linuxfoundation.org" + + +def _install_ssh_config( + monkeypatch: pytest.MonkeyPatch, config_path: Path, user: str +) -> None: + """Point ``SSHConfig()`` at a config file naming ``user``. + + ``get_ssh_user_for_gerrit`` keys its cache on ``~/.ssh/config`` as a + path string, so the key is the same for both tests below regardless + of where the file they parse actually lives. + """ + config_path.write_text( + f"Host {_SHARED_HOST}\n User {user}\n", encoding="utf-8" + ) + + def _factory() -> SSHConfig: + return SSHConfig(config_path=config_path) + + monkeypatch.setattr(ssh_mod, "SSHConfig", _factory, raising=True) + + +def test_ssh_config_cache_polluter_parses_a_config_file( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Parse and cache an SSH config naming a distinctive user.""" + _install_ssh_config(monkeypatch, tmp_path / "config", "polluter") + + assert ssh_mod.get_ssh_user_for_gerrit(_SHARED_HOST) == "polluter" + assert ssh_mod._ssh_config_cache + + +def test_ssh_config_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A later test parses its own config rather than the cached one. + + The cache is keyed on the config path and never on that file's + contents, so without the reset the previous test's parsed instance + is returned and this test never sees its own config at all. + """ + _install_ssh_config(monkeypatch, tmp_path / "config", "victim") + + assert ssh_mod.get_ssh_user_for_gerrit(_SHARED_HOST) == "victim" diff --git a/tests/test_cli.py b/tests/test_cli.py index b25b156..46bb880 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -14,23 +14,15 @@ from github2gerrit.cli import app -# Try to use mix_stderr=False for better error separation, but fall back to default -# if the parameter is not supported in this version of typer. -# The mix_stderr parameter was added in typer 0.9.0+ (requires click 8.1+). -# Older versions of typer/click will raise TypeError for unknown parameters. -try: - runner = CliRunner(mix_stderr=False) -except TypeError: - runner = CliRunner() +# The test suite requires click >= 8.2, pinned in the dev extra, where +# CliRunner always captures stderr separately and the mix_stderr +# parameter no longer exists. +runner = CliRunner() def _get_combined_output(result: Any) -> str: - """Get combined stdout and stderr, handling version differences.""" - try: - return result.stdout + (result.stderr or "") - except ValueError: - # stderr not separately captured, use stdout only - return result.stdout + """Get combined stdout and stderr from an invocation result.""" + return result.stdout + result.stderr def test_conflicting_options_error_message_in_stderr(tmp_path: Path) -> None: diff --git a/tests/test_composite_action_coverage.py b/tests/test_composite_action_coverage.py index a809293..1659039 100644 --- a/tests/test_composite_action_coverage.py +++ b/tests/test_composite_action_coverage.py @@ -354,6 +354,9 @@ def test_github_context_mapping(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) github_vars = [ @@ -396,6 +399,9 @@ def test_issue_id_priority(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) issue_id_env = env_mapping.get("ISSUE_ID", "") issue_id_lookup_json_env = env_mapping.get("ISSUE_ID_LOOKUP_JSON", "") @@ -642,6 +648,9 @@ def test_github_token_usage(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) assert env_mapping.get("GITHUB_TOKEN") == "${{ github.token }}" diff --git a/tests/test_config_helpers.py b/tests/test_config_helpers.py index af5f67e..e243876 100644 --- a/tests/test_config_helpers.py +++ b/tests/test_config_helpers.py @@ -22,7 +22,9 @@ from github2gerrit.config import apply_parameter_derivation from github2gerrit.config import derive_gerrit_parameters from github2gerrit.config import filter_known +from github2gerrit.config import is_derived_key from github2gerrit.config import load_org_config +from github2gerrit.config import mark_derived_keys from github2gerrit.config import overlay_missing @@ -874,3 +876,263 @@ def test_sanitize_ssh_key_content_strips_embedded_whitespace() -> None: assert parts[1] == "b3BlbnNzaC1rZXktdjE" assert parts[2] == "AAAABBBBCCCC" assert parts[3] == "-----END OPENSSH PRIVATE KEY-----" + + +def test_mark_derived_keys_round_trip() -> None: + """Marked keys report as derived; unmarked ones do not.""" + assert not is_derived_key("GERRIT_PROJECT") + + mark_derived_keys(["GERRIT_PROJECT"]) + + assert is_derived_key("GERRIT_PROJECT") + # Key names are normalised, so callers need not match case. + assert is_derived_key("gerrit_project") + assert is_derived_key(" GERRIT_PROJECT ") + # An unrelated key is unaffected. + assert not is_derived_key("GERRIT_SERVER") + + +def test_mark_derived_keys_is_additive() -> None: + """A second call must not discard the first call's record.""" + mark_derived_keys(["GERRIT_SERVER"]) + mark_derived_keys(["GERRIT_PROJECT"]) + + assert is_derived_key("GERRIT_SERVER") + assert is_derived_key("GERRIT_PROJECT") + + # Empty and whitespace-only names are ignored rather than recorded. + mark_derived_keys(["", " "]) + assert is_derived_key("GERRIT_SERVER") + assert not is_derived_key("") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_marks_only_unset_env_keys( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Only keys that apply_config_to_env actually writes are marked. + + apply_config_to_env skips keys whose environment value is already + non-empty, so such a key keeps its explicit value and must not be + described as derived. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + # Setting these through monkeypatch registers them for teardown even + # though apply_config_to_env writes os.environ directly. + monkeypatch.setenv("GERRIT_SERVER", "explicit.gerrit.example.org") + monkeypatch.setenv("GERRIT_PROJECT", "") + monkeypatch.setenv("GERRIT_SSH_USER_G2G", "") + monkeypatch.setenv("GERRIT_SSH_USER_G2G_EMAIL", "") + + cfg = apply_parameter_derivation( + {}, "onap", repository="onap/integration-distribution" + ) + apply_config_to_env(cfg) + + # Derivation supplied a host, but the environment already held one, + # so the explicit value survives and provenance stays explicit. + assert os.environ["GERRIT_SERVER"] == "explicit.gerrit.example.org" + assert not is_derived_key("GERRIT_SERVER") + + # The project had no explicit value, so the guess landed and is + # recorded as derived. + assert os.environ["GERRIT_PROJECT"] == "integration-distribution" + assert is_derived_key("GERRIT_PROJECT") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_can_skip_marking( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """mark_derived=False records nothing. + + Used by call sites that only persist derived values to the + configuration file and never export them to the environment. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_SERVER", "") + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {}, + "onap", + repository="onap/sandbox", + save_to_config=False, + mark_derived=False, + ) + + # Derivation still happened; only the provenance record is skipped. + assert result["GERRIT_PROJECT"] == "sandbox" + assert not is_derived_key("GERRIT_PROJECT") + assert not is_derived_key("GERRIT_SERVER") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_marks_config_file_project( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A GERRIT_PROJECT already in cfg is a fallback, not intent. + + Releases that auto-saved derived values wrote them to the + configuration file, whose sections are per-organization while a + project name is per-repository. On a later run load_org_config + hands that name back in cfg, so it is never missing, never enters + newly_derived, and nothing above records it -- leaving an old + repository-name guess indistinguishable from operator intent. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + ) + + assert is_derived_key("GERRIT_PROJECT") + + # Demoted, not discarded: a misscoped project still scopes the + # Gerrit query, which beats searching every project on the server + # whenever no .gitreview resolves. + assert result["GERRIT_PROJECT"] == "stale-org-project" + apply_config_to_env(result) + assert os.environ["GERRIT_PROJECT"] == "stale-org-project" + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_keeps_explicit_env_project( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An environment GERRIT_PROJECT keeps its explicit provenance. + + Unlike the per-organization file, the environment is scoped to this + run, so an operator naming a project there means it for this + repository. apply_config_to_env leaves such a value untouched, and + the provenance record must agree with what it actually wrote. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "operator/project") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + ) + apply_config_to_env(result) + + assert os.environ["GERRIT_PROJECT"] == "operator/project" + assert not is_derived_key("GERRIT_PROJECT") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_config_project_opt_out( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """mark_derived=False covers the configuration-file project too. + + The opt-out serves call sites that never follow up with + apply_config_to_env, so no key they handle may be described as + holding a derived value there -- whatever its source. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + mark_derived=False, + ) + + assert result["GERRIT_PROJECT"] == "stale-org-project" + assert not is_derived_key("GERRIT_PROJECT") + + +def test_save_derived_parameters_omits_per_repository_project( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """GERRIT_PROJECT never reaches the per-organization section. + + The section is shared by every repository in the organization, so + persisting one repository's project name would misroute all the + others. It would also return on the next run indistinguishable + from operator intent, defeating the provenance tracking that lets + duplicate detection prefer a per-pull-request .gitreview. + """ + from github2gerrit.config import save_derived_parameters_to_config + + config_file = tmp_path / "config.txt" + config_file.write_text( + '[default]\nGERRIT_HTTP_USER = "existing_user"\n', encoding="utf-8" + ) + + monkeypatch.setenv("G2G_CONFIG_PATH", str(config_file)) + monkeypatch.delenv("DRY_RUN", raising=False) + + save_derived_parameters_to_config( + "onap", + { + "GERRIT_SERVER": "gerrit.onap.org", + "GERRIT_PROJECT": "integration/distribution", + "GERRIT_SSH_USER_G2G": "onap.gh2gerrit", + }, + ) + + updated_content = config_file.read_text(encoding="utf-8") + assert 'GERRIT_SERVER = "gerrit.onap.org"' in updated_content + assert 'GERRIT_SSH_USER_G2G = "onap.gh2gerrit"' in updated_content + assert "GERRIT_PROJECT" not in updated_content + assert "integration/distribution" not in updated_content + + +def test_save_derived_parameters_project_only_writes_nothing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Filtering out the only parameter must leave the file untouched. + + The file has to exist for this to prove anything: the function + already returns early when it does not, so a missing file would + make the assertion pass for the wrong reason. + """ + from github2gerrit.config import save_derived_parameters_to_config + + config_file = tmp_path / "config.txt" + original_content = '[default]\nGERRIT_HTTP_USER = "existing_user"\n' + config_file.write_text(original_content, encoding="utf-8") + + monkeypatch.setenv("G2G_CONFIG_PATH", str(config_file)) + monkeypatch.delenv("DRY_RUN", raising=False) + + save_derived_parameters_to_config( + "onap", {"GERRIT_PROJECT": "integration/distribution"} + ) + + assert config_file.read_text(encoding="utf-8") == original_content diff --git a/tests/test_core_integration_fixture_repo.py b/tests/test_core_integration_fixture_repo.py index 4273d76..bf7174f 100644 --- a/tests/test_core_integration_fixture_repo.py +++ b/tests/test_core_integration_fixture_repo.py @@ -36,7 +36,7 @@ def _minimal_inputs(*, dry_run: bool = False) -> Inputs: dry_run=dry_run, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_core_prepare_commits.py b/tests/test_core_prepare_commits.py index 1f6ae5f..1ba9654 100644 --- a/tests/test_core_prepare_commits.py +++ b/tests/test_core_prepare_commits.py @@ -30,7 +30,7 @@ def _minimal_inputs() -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_core_ssh_setup.py b/tests/test_core_ssh_setup.py index 2c44d7a..3a5eb82 100644 --- a/tests/test_core_ssh_setup.py +++ b/tests/test_core_ssh_setup.py @@ -87,7 +87,7 @@ def minimal_inputs() -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -185,7 +185,7 @@ def test_ssh_setup_skips_when_credentials_missing(tmp_path: Path) -> None: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", @@ -348,7 +348,7 @@ def test_ssh_auto_discovery_integration( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -417,7 +417,7 @@ def test_ssh_auto_discovery_fallback_when_discovery_fails( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -481,7 +481,7 @@ def test_ssh_setup_augments_provided_known_hosts_with_autodiscovery( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_duplicate_detection.py b/tests/test_duplicate_detection.py index d0a6190..8de1e2d 100644 --- a/tests/test_duplicate_detection.py +++ b/tests/test_duplicate_detection.py @@ -3,6 +3,8 @@ """Tests for duplicate change detection.""" +import os +import re from datetime import UTC from datetime import datetime from datetime import timedelta @@ -10,10 +12,16 @@ from typing import Any from unittest.mock import Mock from unittest.mock import patch +from urllib.error import URLError +from urllib.parse import unquote +from urllib.parse import urlsplit import pytest import responses +from github2gerrit.config import apply_config_to_env +from github2gerrit.config import apply_parameter_derivation +from github2gerrit.config import mark_derived_keys from github2gerrit.duplicate_detection import ChangeFingerprint from github2gerrit.duplicate_detection import DuplicateChangeError from github2gerrit.duplicate_detection import DuplicateDetector @@ -21,6 +29,68 @@ from github2gerrit.models import GitHubContext +def _gitreview_urlopen(content: bytes | None) -> Any: + """Build a ``urllib.request.urlopen`` side effect for tests. + + Serves *content* only for an ``https://raw.githubusercontent.com`` + origin and refuses everything else, so an unexpected network call + fails loudly rather than resolving through the real network. The + origin is compared after parsing rather than by substring, since a + substring test would also accept + ``https://raw.githubusercontent.com.example.invalid/...`` or a URL + carrying the name only in its path or query string. Pass ``None`` + to make the ``.gitreview`` fetch itself unresolvable. + """ + + def _open(url: Any, *_args: Any, **_kwargs: Any) -> Any: + target = url if isinstance(url, str) else getattr(url, "full_url", "") + parsed = urlsplit(target) + from_raw_github = ( + parsed.scheme == "https" + and parsed.netloc == "raw.githubusercontent.com" + ) + if content is not None and from_raw_github: + response = Mock() + response.read.return_value = content + response.__enter__ = Mock(return_value=response) + response.__exit__ = Mock(return_value=None) + return response + raise URLError(f"blocked in test: {target}") + + return _open + + +@pytest.mark.parametrize( + "url", + [ + "https://raw.githubusercontent.com.example.invalid/o/r/b/.gitreview", + "https://example.invalid/?raw.githubusercontent.com", + "https://example.invalid/raw.githubusercontent.com/.gitreview", + "http://raw.githubusercontent.com/o/r/b/.gitreview", + ], +) +def test_gitreview_urlopen_rejects_lookalike_origins(url: str) -> None: + """The test guard matches the origin, not merely a substring. + + A substring check would serve mocked content to a lookalike host, a + path segment or a query string, so a malformed or redirected fetch + URL could pass these tests silently. + """ + opener = _gitreview_urlopen(b"[gerrit]\nhost=gerrit.example.org\n") + + with pytest.raises(URLError): + opener(url) + + +def test_gitreview_urlopen_accepts_expected_origin() -> None: + """The guard still serves the origin the fetcher actually uses.""" + opener = _gitreview_urlopen(b"[gerrit]\nhost=gerrit.example.org\n") + + response = opener("https://raw.githubusercontent.com/o/r/main/.gitreview") + + assert b"gerrit.example.org" in response.read() + + class TestChangeFingerprint: """Test ChangeFingerprint functionality.""" @@ -381,6 +451,391 @@ def test_resolve_gerrit_info_from_remote_gitreview( result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) assert result == ("gerrit.example.org", "test/project") + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_explicit_env_skips_gitreview( + self, mock_urlopen: Any + ) -> None: + """Explicit configuration short-circuits before any fetch. + + Values the operator set deliberately outrank the pull request's + ``.gitreview``, and resolving them must cost no network I/O. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.wrong.org\nproject=wrong/project\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.example.org", + "GERRIT_PROJECT": "test/project", + }, + ): + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.example.org", "test/project") + mock_urlopen.assert_not_called() + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_gitreview_outranks_derived_env( + self, mock_urlopen: Any + ) -> None: + """A derived environment pair loses to a resolvable .gitreview. + + Derivation guesses the project from the GitHub repository name, + so the per-pull-request ``.gitreview`` is the better answer. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.real.org\nport=29418\n" + b"project=real/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.real.org", "real/project") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_keeps_explicit_host_takes_project( + self, mock_urlopen: Any + ) -> None: + """A derived project must not drag an explicit host down with it. + + Provenance is per field. Treating the pair as a unit made one + derived key demote both, so ``.gitreview`` replaced a host the + operator had configured deliberately. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=gitreview/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.explicit.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.explicit.org", "gitreview/project") + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + @patch("urllib.request.urlopen") + def test_legacy_config_file_project_loses_to_gitreview( + self, + mock_urlopen: Any, + mock_derive_creds: Any, + mock_config_gitreview_host: Any, + ) -> None: + """The same outcome, reached through the configuration file. + + The case above marks the project derived directly. This one + earns the mark the way a real local CLI run does: releases that + auto-saved derived values put GERRIT_PROJECT in the + per-organization section, so load_org_config returns an old + repository-name guess, derivation finds the key already filled, + and without provenance duplicate detection would short-circuit + on it and query the wrong project. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="onap/integration-distribution" + ) + + # Keep derivation off the SSH config and off the network. + mock_derive_creds.return_value = (None, None) + mock_config_gitreview_host.return_value = None + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=integration/distribution.git\n" + ) + + with patch.dict( + "os.environ", + { + # Set by the operator for this run. + "GERRIT_SERVER": "gerrit.explicit.org", + # Unset, as on a local CLI run that leans on the + # per-organization configuration file. + "GERRIT_PROJECT": "", + }, + ): + cfg = apply_parameter_derivation( + # As load_org_config returns it from the stored section. + {"GERRIT_PROJECT": "integration-distribution"}, + "onap", + repository=gh.repository, + save_to_config=False, + ) + apply_config_to_env(cfg) + + # The stale name is in play, alongside the explicit host. + assert os.environ["GERRIT_SERVER"] == "gerrit.explicit.org" + assert os.environ["GERRIT_PROJECT"] == "integration-distribution" + + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.explicit.org", "integration/distribution") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_keeps_explicit_project_takes_host( + self, mock_urlopen: Any + ) -> None: + """The mirror case: a derived host must not discard an explicit + project. + + Only the field lacking operator intent may come from + ``.gitreview``. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=gitreview/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "explicit/project", + }, + ): + mark_derived_keys(["GERRIT_SERVER"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.gitreview.org", "explicit/project") + + @patch("urllib.request.urlopen") + def test_host_only_gitreview_keeps_derived_project_in_query( + self, mock_urlopen: Any + ) -> None: + """A ``.gitreview`` without ``project=`` must not blank the + project. + + ``parse_gitreview`` needs only ``host=``, so a host-only file + parses successfully and leaves ``info.project`` empty. Taking + that empty string over the derived environment value drops the + ``project:`` qualifier from the Gerrit query, which then + searches every project on the server: an unrelated change that + happens to share a title would block a legitimate submission. + A derived project is a guess, but a scoped guess beats an + unscoped search. + """ + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + ) + + responses.start() + try: + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="org/derived-project" + ) + + target_pr = Mock() + target_pr.number = 123 + target_pr.title = "Fix authentication" + target_pr.body = "" + target_pr.get_files.return_value = [] + + responses.add( + responses.GET, + re.compile(r"https://gerrit\.gitreview\.org/.*"), + json=[], + status=200, + ) + + with patch.dict( + "os.environ", + { + # As parameter derivation would leave them. + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + # Pin the base path so no discovery probe runs. + "GERRIT_HTTP_BASE_PATH": "r", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + + resolved = detector._resolve_gerrit_info_from_env_or_gitreview( + gh + ) + assert resolved == ( + "gerrit.gitreview.org", + "derived-project", + ) + + detector.check_for_duplicates( + target_pr, allow_duplicates=False, gh=gh + ) + + queried = [ + unquote(call.request.url or "") for call in responses.calls + ] + assert queried, "expected at least one Gerrit query" + assert all("project:derived-project " in url for url in queried), ( + f"Gerrit query was not scoped to a project: {queried}" + ) + finally: + responses.stop() + responses.reset() + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_falls_back_to_derived_env( + self, mock_urlopen: Any + ) -> None: + """An unresolvable .gitreview leaves the derived pair in play. + + Returning None instead would turn a possibly-wrong query into no + duplicate detection at all, which is strictly worse. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen(None) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.derived.org", "derived-project") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_falls_back_without_repository( + self, mock_urlopen: Any + ) -> None: + """With no repository to fetch from, the derived pair still wins + over returning nothing.""" + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context(repository="") + + mock_urlopen.side_effect = _gitreview_urlopen(None) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.derived.org", "derived-project") + mock_urlopen.assert_not_called() + + @patch("urllib.request.urlopen") + def test_duplicate_query_uses_gitreview_path_not_derived_name( + self, mock_urlopen: Any + ) -> None: + """Acceptance criterion for a Gerrit project with a path separator. + + ``opendaylight/integration-distribution`` derives the project + ``integration-distribution``, but Gerrit hosts it at + ``integration/distribution``. Querying the derived name asks + about a project that does not exist, so every duplicate is + silently missed. The ``.gitreview`` must win. + """ + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.opendaylight.org\nport=29418\n" + b"project=integration/distribution.git\n" + ) + + responses.start() + try: + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="opendaylight/integration-distribution" + ) + + target_pr = Mock() + target_pr.number = 123 + target_pr.title = "Fix authentication" + target_pr.body = "" + target_pr.get_files.return_value = [] + + # Answer any query, so the only thing that can distinguish + # the two projects is the query Gerrit was actually asked. + responses.add( + responses.GET, + re.compile(r"https://gerrit\.opendaylight\.org/.*"), + json=[ + { + "_number": 12345, + "subject": "Fix authentication", + "project": "integration/distribution", + "current_revision": "abc123", + "revisions": { + "abc123": { + "commit": {"message": "Fix authentication"}, + "files": {}, + } + }, + } + ], + status=200, + ) + + with patch.dict( + "os.environ", + { + # As parameter derivation would leave them. + "GERRIT_SERVER": "gerrit.opendaylight.org", + "GERRIT_PROJECT": "integration-distribution", + # Pin the base path so no discovery probe runs. + "GERRIT_HTTP_BASE_PATH": "r", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + + with pytest.raises(DuplicateChangeError): + detector.check_for_duplicates( + target_pr, allow_duplicates=False, gh=gh + ) + + queried = [ + unquote(call.request.url or "") for call in responses.calls + ] + assert queried, "expected at least one Gerrit query" + assert any( + "project:integration/distribution" in url for url in queried + ), f"Gerrit was not queried for the .gitreview project: {queried}" + assert not any( + "project:integration-distribution" in url for url in queried + ), f"Gerrit was queried for the derived project: {queried}" + finally: + responses.stop() + responses.reset() + def test_check_for_duplicates_with_gerrit_duplicate( self, ) -> None: diff --git a/tests/test_gerrit_change_id_footer.py b/tests/test_gerrit_change_id_footer.py index df4e1a7..1e7e49b 100644 --- a/tests/test_gerrit_change_id_footer.py +++ b/tests/test_gerrit_change_id_footer.py @@ -57,7 +57,7 @@ def _inputs(*, use_pr_as_commit: bool = True) -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_gerrit_pr_closer.py b/tests/test_gerrit_pr_closer.py index df69336..f9a0ae7 100644 --- a/tests/test_gerrit_pr_closer.py +++ b/tests/test_gerrit_pr_closer.py @@ -173,6 +173,7 @@ def test_handles_numeric_pr_numbers(self): result = parse_pr_url(url) + assert result is not None, "Expected a parsed (owner, repo, num)" assert result == ("test", "test", 99999) assert isinstance(result[2], int) diff --git a/tests/test_gerrit_urls_more.py b/tests/test_gerrit_urls_more.py index 47a30ce..26c25aa 100644 --- a/tests/test_gerrit_urls_more.py +++ b/tests/test_gerrit_urls_more.py @@ -12,17 +12,6 @@ from github2gerrit.gerrit_urls import create_gerrit_url_builder -@pytest.fixture(autouse=True) -def clear_cache_between_tests() -> None: - """Clear the base path cache between tests to prevent pollution.""" - _clear_builder_cache() - - -def _clear_builder_cache() -> None: - # Ensure base-path discovery cache does not bleed across tests - urls_mod._BASE_PATH_CACHE.clear() # pyright: ignore[reportPrivateUsage] - - class _FakeResp: def __init__( self, code: int, headers: dict[str, str] | None = None @@ -206,9 +195,13 @@ def decide(url: str) -> _FakeResp | BaseException: assert b1.base_path == "r" assert b1.web_url("dashboard").startswith("https://gerrit.example.org/r/") - # Now simulate absolute URL in Location header - # Cache is cleared automatically by the autouse fixture between test sections - # when we create a new builder + # Now simulate absolute URL in Location header. This clear is + # deliberate and mid-test: the first half cached 'r' for this host, + # and without discarding it the second builder would short-circuit + # on the cache and never reach decide_abs, leaving the + # absolute-Location branch untested and the assertions below + # vacuously true. The autouse reset in conftest.py runs around the + # test, not inside it, so it cannot do this. urls_mod._BASE_PATH_CACHE.clear() # pyright: ignore[reportPrivateUsage] def decide_abs(url: str) -> _FakeResp | BaseException: diff --git a/tests/test_mapping_comment_additional.py b/tests/test_mapping_comment_additional.py index 11a0ba5..e0180d5 100644 --- a/tests/test_mapping_comment_additional.py +++ b/tests/test_mapping_comment_additional.py @@ -298,30 +298,45 @@ def test_reconciliation_all_new_ids_no_topic_no_comment(monkeypatch): is enabled, all commits receive brand new Change-Ids. """ from types import SimpleNamespace + from typing import cast + from github2gerrit.gitreview import GerritInfo + from github2gerrit.models import GitHubContext + from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.reconcile_matcher import LocalCommit - class GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 9 - - class Gerrit: - host = "gerrit.example.org" - port = 29418 + gh = GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=9, + ) + gerrit = GerritInfo(host="gerrit.example.org", port=29418) # Force empty topic results from github2gerrit.orchestrator import reconciliation as rmod monkeypatch.setattr(rmod, "query_changes_by_topic", lambda *a, **k: []) - inputs = SimpleNamespace( - reuse_strategy="topic", - allow_orphan_changes=False, - similarity_subject=0.7, - log_reconcile_json=False, + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + inputs = cast( + Inputs, + SimpleNamespace( + reuse_strategy="topic", + allow_orphan_changes=False, + similarity_subject=0.7, + log_reconcile_json=False, + ), ) commits = [ LocalCommit( @@ -336,8 +351,8 @@ class Gerrit: ] result = perform_reconciliation( inputs=inputs, - gh=GH(), - gerrit=Gerrit(), + gh=gh, + gerrit=gerrit, local_commits=commits, ) assert len(result) == 3 @@ -351,31 +366,43 @@ def test_reconciliation_topic_query_exception(monkeypatch): (fallback path). """ from types import SimpleNamespace + from typing import cast + from github2gerrit.gitreview import GerritInfo + from github2gerrit.models import GitHubContext + from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.orchestrator import reconciliation as rmod from github2gerrit.reconcile_matcher import LocalCommit - class GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 11 - - class Gerrit: - host = "gerrit.example.org" - port = 29418 + gh = GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=11, + ) + gerrit = GerritInfo(host="gerrit.example.org", port=29418) def _boom(*_a, **_k): raise RuntimeError("boom") monkeypatch.setattr(rmod, "query_changes_by_topic", _boom) - inputs = SimpleNamespace( - reuse_strategy="topic", - allow_orphan_changes=False, - similarity_subject=0.7, - log_reconcile_json=False, + inputs = cast( + Inputs, + SimpleNamespace( + reuse_strategy="topic", + allow_orphan_changes=False, + similarity_subject=0.7, + log_reconcile_json=False, + ), ) commits = [ LocalCommit( @@ -389,8 +416,8 @@ def _boom(*_a, **_k): ] result = perform_reconciliation( inputs=inputs, - gh=GH(), - gerrit=Gerrit(), + gh=gh, + gerrit=gerrit, local_commits=commits, ) assert len(result) == 1 diff --git a/tests/test_mapping_comment_digest_and_backref.py b/tests/test_mapping_comment_digest_and_backref.py index 332354b..e4ea974 100644 --- a/tests/test_mapping_comment_digest_and_backref.py +++ b/tests/test_mapping_comment_digest_and_backref.py @@ -64,14 +64,13 @@ def test_idempotent_backref_skip_and_force( the back-reference for that commit should be skipped unless forced. """ # Prepare orchestrator with minimal required attributes - orch = Orchestrator(workspace=str(tmp_path)) - orch.workspace = str(tmp_path) + orch = Orchestrator(workspace=tmp_path) # Fake GitHub context gh = GitHubContext( event_name="pull_request_target", event_action="synchronize", - event_path="", + event_path=None, repository="org/repo", repository_owner="org", server_url="https://github.com", diff --git a/tests/test_orphan_rest_side_effects.py b/tests/test_orphan_rest_side_effects.py index 696423b..3ee07da 100644 --- a/tests/test_orphan_rest_side_effects.py +++ b/tests/test_orphan_rest_side_effects.py @@ -9,8 +9,11 @@ from __future__ import annotations +from typing import Any + import pytest +from github2gerrit.gitreview import GerritInfo from github2gerrit.orchestrator.reconciliation import _abandon_orphan_changes from github2gerrit.orchestrator.reconciliation import _comment_orphan_changes @@ -25,7 +28,7 @@ def __init__(self): # an authenticated client that can perform mutating REST calls. self.is_authenticated = True - def post(self, path: str, data: dict | None = None): + def post(self, path: str, data: dict[str, Any] | None = None): """Record POST calls and optionally simulate failures.""" self.post_calls.append((path, data)) @@ -42,11 +45,9 @@ def make_fail(self, path: str, error_msg: str): self.should_fail[path] = error_msg -class MockGerrit: - """Mock Gerrit info object.""" - - def __init__(self, host: str = "gerrit.example.org"): - self.host = host +def _gerrit_info(host: str = "gerrit.example.org") -> GerritInfo: + """Build genuine Gerrit connection info (only ``host`` is read).""" + return GerritInfo(host=host) @pytest.fixture @@ -57,8 +58,8 @@ def mock_client(): @pytest.fixture def mock_gerrit(): - """Provide a mock Gerrit info object.""" - return MockGerrit() + """Provide Gerrit connection info.""" + return _gerrit_info() def test_abandon_orphan_changes_success(mock_client, mock_gerrit, monkeypatch): @@ -202,7 +203,7 @@ def mock_build_client(host, **kwargs): def test_abandon_orphan_changes_no_ids(): """Test abandon with empty orphan list.""" - result = _abandon_orphan_changes([], MockGerrit()) + result = _abandon_orphan_changes([], _gerrit_info()) assert result == [] @@ -216,7 +217,7 @@ def test_abandon_orphan_changes_no_gerrit(): def test_comment_orphan_changes_no_ids(): """Test comment with empty orphan list.""" - result = _comment_orphan_changes([], MockGerrit()) + result = _comment_orphan_changes([], _gerrit_info()) assert result == [] diff --git a/tests/test_pr_commands.py b/tests/test_pr_commands.py index 949f05b..eae7c8d 100644 --- a/tests/test_pr_commands.py +++ b/tests/test_pr_commands.py @@ -414,7 +414,7 @@ def _make_inputs(self, create_missing: bool = False, **kwargs: Any): """Create a minimal Inputs object for testing.""" from github2gerrit.models import Inputs - defaults = { + defaults: dict[str, Any] = { "submit_single_commits": False, "use_pr_as_commit": False, "fetch_depth": 10, @@ -441,7 +441,7 @@ def _make_inputs(self, create_missing: bool = False, **kwargs: Any): defaults.update(kwargs) return Inputs(**defaults) - def _make_gh(self, pr_number: int = 42): + def _make_gh(self, pr_number: int | None = 42): """Create a minimal GitHubContext for testing.""" from github2gerrit.models import GitHubContext diff --git a/tests/test_pr_content_filter_integration.py b/tests/test_pr_content_filter_integration.py index 83b5898..7a7895e 100644 --- a/tests/test_pr_content_filter_integration.py +++ b/tests/test_pr_content_filter_integration.py @@ -33,7 +33,7 @@ def _create_inputs(self, use_pr_as_commit: bool = True) -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="test/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_reconciliation_extracted_module.py b/tests/test_reconciliation_extracted_module.py index 6207496..3637a9a 100644 --- a/tests/test_reconciliation_extracted_module.py +++ b/tests/test_reconciliation_extracted_module.py @@ -14,10 +14,14 @@ import logging from types import SimpleNamespace +from typing import cast import pytest from github2gerrit.gerrit_query import GerritChange +from github2gerrit.gitreview import GerritInfo +from github2gerrit.models import GitHubContext +from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.orchestrator import reconciliation as recon_mod from github2gerrit.reconcile_matcher import LocalCommit @@ -56,16 +60,24 @@ def _gerrit_change( ) -class _DummyGH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 5 +def _gh_context() -> GitHubContext: + return GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=5, + ) -class _DummyGerrit: - host = "gerrit.example.org" - port = 29418 +def _gerrit_info() -> GerritInfo: + return GerritInfo(host="gerrit.example.org", port=29418) def _inputs( @@ -74,12 +86,18 @@ def _inputs( allow_orphan_changes: bool = False, similarity_subject: float = 0.7, log_reconcile_json: bool = False, -): - return SimpleNamespace( - reuse_strategy=reuse_strategy, - allow_orphan_changes=allow_orphan_changes, - similarity_subject=similarity_subject, - log_reconcile_json=log_reconcile_json, +) -> Inputs: + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + return cast( + Inputs, + SimpleNamespace( + reuse_strategy=reuse_strategy, + allow_orphan_changes=allow_orphan_changes, + similarity_subject=similarity_subject, + log_reconcile_json=log_reconcile_json, + ), ) @@ -89,8 +107,8 @@ def _inputs( def test_reconciliation_strategy_none_returns_empty(): - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="none") local_commits = [_local_commit(0, "a1", "feat: one")] result = perform_reconciliation( @@ -103,8 +121,8 @@ def test_reconciliation_strategy_none_returns_empty(): def test_reconciliation_topic_reuse_path(monkeypatch: pytest.MonkeyPatch): - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic") pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "abc123hash" @@ -137,8 +155,8 @@ def test_reconciliation_queries_canonical_topic( monkeypatch: pytest.MonkeyPatch, ): """Topic discovery must query the topic format the push side uses.""" - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic") monkeypatch.delenv("G2G_TOPIC_PREFIX", raising=False) @@ -183,8 +201,8 @@ def test_reconciliation_comment_fallback_extension( When topic path yields no changes and comment mapping exists with fewer entries than local commits, new IDs must extend mapping. """ - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic+comment") # Empty topic result @@ -227,8 +245,8 @@ def test_reconciliation_json_summary_emitted( Enabling log_reconcile_json should emit a RECONCILE_SUMMARY log line. """ caplog.set_level(logging.DEBUG) - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic", log_reconcile_json=True) pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "def456hash" diff --git a/tests/test_reconciliation_plan_and_orphans.py b/tests/test_reconciliation_plan_and_orphans.py index 6fea6db..6fcf770 100644 --- a/tests/test_reconciliation_plan_and_orphans.py +++ b/tests/test_reconciliation_plan_and_orphans.py @@ -18,11 +18,16 @@ import json import re from types import SimpleNamespace +from typing import Any +from typing import cast # (removed unused List import) import pytest from github2gerrit.gerrit_query import GerritChange +from github2gerrit.gitreview import GerritInfo +from github2gerrit.models import GitHubContext +from github2gerrit.models import Inputs from github2gerrit.orchestrator import reconciliation as recon_mod from github2gerrit.orchestrator.reconciliation import perform_reconciliation from github2gerrit.reconcile_matcher import LocalCommit @@ -33,17 +38,24 @@ # --------------------------------------------------------------------------- -class _GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 42 - run_id = "99999" +def _gh_context() -> GitHubContext: + return GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="99999", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=42, + ) -class _Gerrit: - host = "gerrit.example.org" - port = 29418 +def _gerrit_info() -> GerritInfo: + return GerritInfo(host="gerrit.example.org", port=29418) def _lc(idx: int, sha: str, subject: str, files: list[str]) -> LocalCommit: @@ -80,16 +92,22 @@ def _inputs( log_reconcile_json: bool = True, orphan_policy: str = "comment", similarity_subject: float = 0.7, -): - return SimpleNamespace( - reuse_strategy=reuse_strategy, - log_reconcile_json=log_reconcile_json, - orphan_policy=orphan_policy, - similarity_subject=similarity_subject, +) -> Inputs: + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + return cast( + Inputs, + SimpleNamespace( + reuse_strategy=reuse_strategy, + log_reconcile_json=log_reconcile_json, + orphan_policy=orphan_policy, + similarity_subject=similarity_subject, + ), ) -def _extract_summary(caplog) -> dict: +def _extract_summary(caplog) -> dict[str, Any]: """ Find and parse the most recent RECONCILE_SUMMARY json line. """ @@ -104,7 +122,7 @@ def _extract_summary(caplog) -> dict: return json.loads(m.group(1)) -def _extract_orphan_actions(caplog) -> dict | None: +def _extract_orphan_actions(caplog) -> dict[str, Any] | None: lines = [r.message for r in caplog.records if "ORPHAN_ACTIONS" in r.message] if not lines: return None @@ -127,8 +145,8 @@ def test_topic_reuse_and_orphan_policy_comment( Orphan policy 'comment' should log commented array with orphan id. """ caplog.set_level("DEBUG") - gh = _GH() - gerrit = _Gerrit() + gh = _gh_context() + gerrit = _gerrit_info() pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "1234abcdhash" # Local commits -> only first Gerrit change reused. @@ -205,8 +223,8 @@ def test_orphan_policy_variants(policy, expected_field, caplog, monkeypatch): Validate that the orphan action bucket changes with policy. """ caplog.set_level("DEBUG") - gh = _GH() - # (removed unused variable; _Gerrit() passed inline) + gh = _gh_context() + # (removed unused variable; _gerrit_info() passed inline) pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "abcd5678hash" local_commits = [_lc(0, "sha-x", "feat: one", ["x.py"])] @@ -250,7 +268,7 @@ def mock_build_client_for_host(host, **kwargs): _ = perform_reconciliation( inputs=inputs, gh=gh, - gerrit=_Gerrit(), + gerrit=_gerrit_info(), local_commits=local_commits, expected_github_hash=gh_hash, ) @@ -269,8 +287,8 @@ def test_comment_based_reuse_extension_digest(caplog, monkeypatch): appended with new Change-Id; digest reflects both. """ caplog.set_level("DEBUG") - gh = _GH() - gerrit = _Gerrit() + gh = _gh_context() + gerrit = _gerrit_info() existing_ids = ["I1234567890abcdef1234567890abcdef12345678"] # Force empty topic results. monkeypatch.setattr(recon_mod, "query_changes_by_topic", lambda *a, **k: []) diff --git a/tests/test_ssh_artifact_prevention.py b/tests/test_ssh_artifact_prevention.py index 46d2aa8..c7eb952 100644 --- a/tests/test_ssh_artifact_prevention.py +++ b/tests/test_ssh_artifact_prevention.py @@ -151,6 +151,13 @@ def test_ssh_files_use_secure_location( orch1._setup_ssh(mock_inputs, mock_gerrit_info) orch2._setup_ssh(mock_inputs, mock_gerrit_info) + assert orch1._ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir on orch1" + ) + assert orch2._ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir on orch2" + ) + # Verify different secure paths are used assert orch1._ssh_temp_dir != orch2._ssh_temp_dir assert "g2g_ssh_" in str(orch1._ssh_temp_dir) @@ -176,6 +183,9 @@ def test_ssh_cleanup_removes_temp_directory( orch._setup_ssh(mock_inputs, mock_gerrit_info) ssh_temp_dir = orch._ssh_temp_dir + assert ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir to clean up" + ) assert ssh_temp_dir.exists() # Cleanup SSH diff --git a/uv.lock b/uv.lock index ec2b57c..3f18d56 100644 --- a/uv.lock +++ b/uv.lock @@ -395,6 +395,7 @@ dependencies = [ [package.optional-dependencies] dev = [ + { name = "click" }, { name = "coverage", extra = ["toml"] }, { name = "mypy" }, { name = "pytest" }, @@ -408,6 +409,7 @@ dev = [ [package.dev-dependencies] dev = [ + { name = "github2gerrit", extra = ["dev"] }, { name = "hatch-vcs" }, { name = "hatchling" }, ] @@ -415,6 +417,7 @@ dev = [ [package.metadata] requires-dist = [ { name = "click", specifier = ">=8.1.7" }, + { name = "click", marker = "extra == 'dev'", specifier = ">=8.2" }, { name = "coverage", extras = ["toml"], marker = "extra == 'dev'", specifier = ">=7.10.6" }, { name = "cryptography", specifier = ">=48.0.1" }, { name = "git-review", specifier = ">=2.5.0" }, @@ -438,6 +441,7 @@ provides-extras = ["dev"] [package.metadata.requires-dev] dev = [ + { name = "github2gerrit", extras = ["dev"] }, { name = "hatch-vcs", specifier = ">=0.5.0" }, { name = "hatchling", specifier = ">=1.28.0" }, ]