diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 7d6f265..b9f8c5b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -76,8 +76,26 @@ repos: rev: 7ff8d35ae36a7d2b968f2f90b4c723e292e594ee # frozen: v2.3.1 hooks: - id: mypy - files: ^src/.+\.py$ + # Every Python path in the repository, matching the ruff and + # basedpyright hooks above and below so the three cannot drift. + files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$ + # mypy runs in an isolated virtualenv that holds only what is + # listed here, so anything the checked files import has to be + # present or mypy draws conclusions the project never would. With + # pytest absent, for instance, @pytest.fixture reads as an untyped + # decorator and pytest.raises stops telling mypy the block can + # swallow the exception, which turns every statement after a + # `with pytest.raises(...)` into dead code. Installing the real + # packages removes that whole class of environment-only findings; + # keep this list in step with the imports under src/ and tests/. + # pygerrit2 is deliberately absent: it is an optional runtime + # dependency and pyproject.toml already gives it + # ignore_missing_imports. additional_dependencies: + - click>=8.2 + - pytest + - responses + - typer - types-PyYAML - types-requests diff --git a/pyproject.toml b/pyproject.toml index dc85d59..445faeb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -119,6 +119,7 @@ dev = [ # Type checking helpers "pytest-mock>=3.15.1", + "types-PyYAML>=6.0.12", "types-requests>=2.31.0", "types-urllib3>=1.26.25.14", ] @@ -166,40 +167,49 @@ warn_unreachable = true warn_unused_ignores = true disallow_any_generics = true disallow_subclassing_any = true -disallow_untyped_defs = true -disallow_incomplete_defs = true -disallow_untyped_calls = true +# Annotation rules are relaxed here and switched back on for the shipped +# package by the github2gerrit.* override below. The obvious shape -- +# global strictness plus a tests.* relaxation -- cannot work, because +# neither half of it matches: +# +# * tests/ has no __init__.py, so mypy names its modules test_cli, +# conftest and so on, never tests.test_cli. +# * A pattern cannot recover the real names either. mypy accepts "*" +# only as a whole dotted component, so "test_*" is rejected outright +# ("Patterns must be fully-qualified module names, optionally with +# '*' in some components") and the section is discarded in silence. +# +# Inverting the default is the only form that both applies and keeps +# applying when test files are added. It also suits scripts/, which is +# unannotated by design. +disallow_untyped_defs = false +disallow_incomplete_defs = false +disallow_untyped_calls = false no_implicit_optional = true show_error_codes = true pretty = true -exclude = [ - "build/", - "dist/", - "docs/", - "scripts/", - "tests/fixtures/", - "tests/test_composite_action_coverage.py", - "tests/test_action_environment_mapping.py", - "tests/test_action_step_validation.py", - "tests/test_action_outputs.py", - "tests/test_trailers_additional.py", -] - +# exclude skips directory discovery only; files named explicitly on the +# command line are still checked. The pre-commit hook passes filenames, +# so anything listed here would be checked there regardless -- keep this +# to paths that hold no first-party Python. +exclude = ["build/", "dist/", "docs/"] + +# The shipped package is held to full strict-mode annotation rules. +# Everything else -- tests/, scripts/, sitecustomize.py -- keeps every +# correctness check (check_untyped_defs, warn_unreachable, +# strict_equality, no_implicit_reexport) but is not required to annotate +# signatures. This mirrors the basedpyright stance recorded in +# [[tool.pyright.executionEnvironments]] below, where #392 disabled +# reportMissingParameterType for tests/ because pytest already resolves +# fixtures by parameter name. [[tool.mypy.overrides]] -module = [ - "tests.test_composite_action_coverage", - "tests.test_action_environment_mapping", - "tests.test_action_step_validation", - "tests.test_action_outputs", - "tests.test_trailers_additional", -] -disallow_untyped_defs = false -disallow_incomplete_defs = false -disallow_untyped_calls = false -check_untyped_defs = false +module = ["github2gerrit.*"] +disallow_untyped_defs = true +disallow_incomplete_defs = true +disallow_untyped_calls = true [[tool.mypy.overrides]] -module = ["pygerrit2", "pygerrit2.*"] +module = ["pygerrit2"] ignore_missing_imports = true [[tool.mypy.overrides]] @@ -210,21 +220,12 @@ ignore_missing_imports = true module = ["typer", "typer.*"] ignore_missing_imports = true -[[tool.mypy.overrides]] -module = ["pytest", "pytest.*"] -ignore_missing_imports = true - [[tool.mypy.overrides]] module = ["github2gerrit.rich_display"] disallow_untyped_defs = false disallow_untyped_calls = false check_untyped_defs = false -[[tool.mypy.overrides]] -module = ["tests.*"] -disallow_untyped_defs = false -disallow_untyped_calls = false - [tool.coverage.run] branch = true # Keep transient coverage data out of the working tree so tests that diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index f8463c6..f746503 100644 --- a/src/github2gerrit/cli.py +++ b/src/github2gerrit/cli.py @@ -30,7 +30,6 @@ from urllib.parse import urlparse import click -import click.core import typer from . import models @@ -762,10 +761,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( @@ -810,7 +817,13 @@ def _resolve_bool_override( provided on the command line, so CLI flags always take precedence. """ source = ctx.get_parameter_source(param_name) # pyright: ignore[reportAttributeAccessIssue] - if source == click.core.ParameterSource.COMMANDLINE: # pyright: ignore[reportAttributeAccessIssue, reportUnnecessaryComparison] + # Compare by member name rather than by identity with a ParameterSource + # member. typer >= 0.20 ships its own copy of click under typer._click, + # so ctx.get_parameter_source returns a + # typer._click.core.ParameterSource member, which never compares equal + # to the click.core.ParameterSource member of the same name. Matching + # on .name is correct whichever copy typer hands back. + if source is not None and source.name == "COMMANDLINE": return current env_val = os.getenv(env_var) if env_val is not None: 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/core.py b/src/github2gerrit/core.py index 9a47abf..722c064 100644 --- a/src/github2gerrit/core.py +++ b/src/github2gerrit/core.py @@ -63,7 +63,7 @@ from .github_api import get_recent_change_ids_from_comments from .github_api import get_repo_from_env from .github_api import iter_open_pulls -from .gitreview import GerritInfo +from .gitreview import GerritInfo as GerritInfo from .gitreview import fetch_gitreview from .gitreview import make_gitreview_info from .gitutils import CommandError @@ -273,8 +273,9 @@ def _is_valid_change_id(value: str) -> bool: # GerritInfo is imported from .gitreview (aliased from GitReviewInfo). # It retains the same frozen-dataclass interface (host, port, project) plus # an additional optional ``base_path`` field (default ``None``). -# The top-level import is sufficient for ``from github2gerrit.core import -# GerritInfo`` to keep working — no explicit re-export needed. +# The top-level import uses the redundant ``as GerritInfo`` form so that +# ``from github2gerrit.core import GerritInfo`` keeps working under +# no_implicit_reexport, which strict type checking enables. @dataclass(frozen=True) 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/src/github2gerrit/ssh_config_parser.py b/src/github2gerrit/ssh_config_parser.py index c450685..cffa934 100644 --- a/src/github2gerrit/ssh_config_parser.py +++ b/src/github2gerrit/ssh_config_parser.py @@ -14,16 +14,14 @@ - Resolves canonical hostnames - Extracts User directives for specific hosts - Thread-safe operation -- Caching to prevent repeated expensive operations -Performance optimizations: -- SSH config files are cached to avoid re-parsing -- Environment variable lookups are cached -- Git email lookups are cached to prevent repeated subprocess calls -- Credential derivation results are cached per host/organization - -The caching helps reduce SSH agent prompts and improves performance when -the same credentials are needed multiple times during execution. +Nothing here is cached. Everything credential derivation depends on -- +the G2G_RESPECT_USER_SSH environment variable, the SSH config on disk +and the git identity -- is read afresh on every call, because each of +them can change during a single process (cli._load_effective_inputs +sets the environment variable at runtime). Holding no process-wide +state is also what makes the "thread-safe operation" claim above true +rather than aspirational. """ from __future__ import annotations @@ -31,7 +29,6 @@ import logging import re import subprocess -from functools import lru_cache from pathlib import Path from typing import Any @@ -276,18 +273,19 @@ def _pattern_matches(self, hostname: str, pattern: str) -> bool: return False -# Global cache for SSH config instances to avoid re-parsing -_ssh_config_cache: dict[str, SSHConfig] = {} - - def get_ssh_user_for_gerrit( gerrit_host: str, gerrit_port: int = 29418 ) -> str | None: """Get SSH user for Gerrit host from SSH configuration. - Convenience function to extract SSH user for a Gerrit server - from the user's SSH configuration. Uses caching to avoid - repeatedly parsing the same SSH config file. + Convenience function to extract SSH user for a Gerrit server from + the user's SSH configuration. The file is parsed on every call. + Caching the parse was tried and removed: keying on the path served a + stale parse once the file was rewritten, and keying on stat metadata + still collides when an in-place rewrite preserves size and inode on + a filesystem with coarse timestamps. A correct key means hashing + the contents, which costs more than the parse it saves -- around + 0.2ms for a fifty-host config, once or twice per process. Args: gerrit_host: Gerrit server hostname @@ -296,45 +294,17 @@ def get_ssh_user_for_gerrit( Returns: SSH username if found in config, None otherwise """ - # Use default SSH config path as cache key - default_config_path = Path.home() / ".ssh" / "config" - cache_key = str(default_config_path) - - if cache_key not in _ssh_config_cache: - _ssh_config_cache[cache_key] = SSHConfig() - - config = _ssh_config_cache[cache_key] - return config.get_user_for_host(gerrit_host, gerrit_port) - - -def clear_ssh_config_cache() -> None: - """Clear the SSH config cache. - - This function is useful for testing or when SSH configuration - files have been modified during runtime. - """ - _ssh_config_cache.clear() - - -def clear_credential_cache() -> None: - """Clear all credential-related caches. - - This function clears both the SSH config cache and the LRU caches - for environment settings and git email lookups. Useful for testing - or when configuration changes during runtime. - """ - clear_ssh_config_cache() - _get_respect_user_ssh_setting.cache_clear() - _get_cached_git_user_email.cache_clear() - derive_gerrit_credentials.cache_clear() + return SSHConfig().get_user_for_host(gerrit_host, gerrit_port) -@lru_cache(maxsize=1) def _get_respect_user_ssh_setting() -> bool: - """Cache the G2G_RESPECT_USER_SSH environment variable setting. + """Read the G2G_RESPECT_USER_SSH environment variable. - This prevents repeated environment variable lookups and ensures - consistent behavior throughout the application lifecycle. + Read on every call rather than memoised: cli._load_effective_inputs + sets this variable at runtime when it detects local execution, so a + value captured earlier in the process can disagree with the + environment. The flag selects between two entirely different + credential sources, which makes a stale read a wrong-identity bug. Returns: Boolean value of G2G_RESPECT_USER_SSH environment variable @@ -342,12 +312,14 @@ def _get_respect_user_ssh_setting() -> bool: return env_bool("G2G_RESPECT_USER_SSH") -@lru_cache(maxsize=1) -def _get_cached_git_user_email() -> str | None: - """Cache git user email lookup to avoid repeated subprocess calls. +def _read_git_user_email() -> str | None: + """Read the configured git user email. + + Read on every call rather than memoised, so that a change of git + identity or working directory is reflected in derived credentials. Returns: - Cached git user email if configured, None otherwise + Git user email if configured, None otherwise """ return get_git_user_email() @@ -440,7 +412,6 @@ def get_git_user_email() -> str | None: return None -@lru_cache(maxsize=32) def derive_gerrit_credentials( gerrit_host: str, organization: str, gerrit_port: int = 29418 ) -> tuple[str | None, str | None]: @@ -452,6 +423,12 @@ def derive_gerrit_credentials( 2. Git user email from local git configuration 3. Fallback to organization-based derivation + Deliberately not memoised. The result depends on + G2G_RESPECT_USER_SSH, on ~/.ssh/config and on the git identity, none + of which are arguments, so a cache keyed on the arguments alone + could serve a personal identity where the organization service + account was asked for. + Args: gerrit_host: Gerrit server hostname organization: GitHub organization name for fallback @@ -469,7 +446,6 @@ def derive_gerrit_credentials( ) # Check if we should respect user SSH config (local mode) - # Use cached environment variable to prevent repeated lookups respect_user_ssh = _get_respect_user_ssh_setting() ssh_user = None @@ -478,8 +454,8 @@ def derive_gerrit_credentials( if respect_user_ssh: # Try SSH config first ssh_user = get_ssh_user_for_gerrit(gerrit_host, gerrit_port) - # Try git config for email (cached to prevent repeated subprocess calls) - git_email = _get_cached_git_user_email() + # Try git config for email + git_email = _read_git_user_email() log.debug( "Local mode: using SSH config user=%s, git email=%s", ssh_user, diff --git a/tests/conftest.py b/tests/conftest.py index efdc1a4..742fe5a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -231,6 +231,35 @@ def reset_gerrit_base_path_cache() -> Iterable[None]: gerrit_urls._BASE_PATH_CACHE.clear() +@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/test_cache_isolation.py b/tests/test_cache_isolation.py new file mode 100644 index 0000000..178797a --- /dev/null +++ b/tests/test_cache_isolation.py @@ -0,0 +1,210 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation + +"""Regression tests for process-wide cache isolation between tests. + +``gerrit_urls._BASE_PATH_CACHE`` memoises expensive lookups in +module-level state that outlives an individual test, and +``tests/conftest.py`` clears it around every test via the +``reset_gerrit_base_path_cache`` autouse fixture. + +``ssh_config_parser`` needs no such fixture. It keeps no process-wide +state at all: credential derivation reads ``G2G_RESPECT_USER_SSH``, the +SSH config and the git identity on every call. The pairs below hold +that to account: each polluter populates whatever state exists with a +value the victim must not see, and each victim asserts the answer it +would get from a clean process. Tests within a module run in +definition order, so the pairing is deterministic. +""" + +from __future__ import annotations + +from collections.abc import Callable +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 + + +# 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: + """Derive credentials from a user-SSH-respecting environment.""" + 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 setting reflects this test's environment, not the last one. + + ``_get_respect_user_ssh_setting`` takes no arguments, so any memo + over it would have exactly one slot that every test in the suite + collides on, over an environment variable tests monkeypatch freely. + It reads the environment on every call instead, so the answer here + is this test's own -- with no fixture clearing anything in between. + """ + 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 reflects this test's git identity. + + ``_read_git_user_email`` is the same shape as + ``_get_respect_user_ssh_setting``: zero arguments, so a memo would + give the whole suite a single shared slot. What it reads is the + output of ``git config user.email``, which ``isolate_git_environment`` + and individual tests both take pains to control, so it is read on + every call and the polluter above cannot reach this test. + """ + monkeypatch.setattr( + ssh_mod, + "get_git_user_email", + lambda: "victim@example.org", + raising=True, + ) + + assert ssh_mod._read_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. + + The polluter above derived personal credentials for this very host, + organisation and port. Were the derivation memoised on those three + arguments its answer would be replayed here, even though it was + computed from an environment this test does not share. Deriving + afresh, this test sees its own empty environment and falls back to + the organisation service account -- no cache clearing required. + """ + 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" diff --git a/tests/test_cli.py b/tests/test_cli.py index 46bb880..727c193 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,10 +6,10 @@ import json import os from pathlib import Path -from typing import Any import pytest from typer.testing import CliRunner +from typer.testing import Result from github2gerrit.cli import app @@ -20,7 +20,7 @@ runner = CliRunner() -def _get_combined_output(result: Any) -> str: +def _get_combined_output(result: Result) -> str: """Get combined stdout and stderr from an invocation result.""" return result.stdout + result.stderr @@ -384,7 +384,7 @@ def test_validation_local_cli_with_derivation_enabled( @pytest.mark.parametrize( "exception_class", [RuntimeError, ValueError, TypeError] -) # type: ignore[misc] # mypy cannot infer the type of exception_class in parametrize when using exception classes +) def test_unexpected_exception_exits_with_code_1( tmp_path: Path, exception_class: type[Exception] ) -> None: diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 4f7119a..51fbb20 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -7,11 +7,14 @@ from pathlib import Path import pytest +import typer from pytest import mark +from typer.testing import CliRunner from github2gerrit.cli import _extract_pr_number from github2gerrit.cli import _mask_secret from github2gerrit.cli import _read_github_context +from github2gerrit.cli import _resolve_bool_override parametrize = mark.parametrize @@ -169,3 +172,74 @@ def test_read_github_context_handles_missing_event_file( assert ctx.base_ref == "" assert ctx.head_ref == "" assert ctx.pr_number is None + + +# --------------------------------------- +# Tests for _resolve_bool_override +# --------------------------------------- + + +def _run_override( + argv: list[str], + env_value: str | None, + monkeypatch: pytest.MonkeyPatch, +) -> bool: + """Invoke a one-option app and report what the override resolved to. + + A real Typer invocation is the only way to exercise this: the + function asks Click which *source* supplied the parameter, and that + is only populated by genuine argument parsing. + """ + if env_value is None: + monkeypatch.delenv("G2G_TEST_FLAG", raising=False) + else: + monkeypatch.setenv("G2G_TEST_FLAG", env_value) + + seen: dict[str, bool] = {} + app = typer.Typer() + + @app.command() + def _cmd( + ctx: typer.Context, + flag: bool = typer.Option(False, "--flag/--no-flag"), + ) -> None: + seen["value"] = _resolve_bool_override( + ctx, "flag", "G2G_TEST_FLAG", flag + ) + + result = CliRunner().invoke(app, argv) + assert result.exit_code == 0, result.output + return seen["value"] + + +@parametrize( + "argv,env_value,expected", + [ + # An explicit flag outranks the environment, in both directions. + (["--flag"], "false", True), + (["--no-flag"], "true", False), + # Without an explicit flag the environment decides. This is the + # case that matters under GitHub Actions, which passes the + # string "false"; Click treats any non-empty string as truthy, + # so the value has to be parsed rather than coerced. + ([], "false", False), + ([], "true", True), + # No flag and no environment variable leaves the default alone. + ([], None, False), + ], +) +def test_resolve_bool_override_precedence( + argv: list[str], + env_value: str | None, + expected: bool, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An explicit CLI flag beats the environment variable behind it. + + Regression test. The check for an explicit flag compared a + ``ParameterSource`` member against ``click.core.ParameterSource``, + but typer >= 0.20 vendors its own copy of click, so the two enum + classes differ and the comparison was never true. Every explicit + flag silently lost to its environment variable. + """ + assert _run_override(argv, env_value, monkeypatch) is expected diff --git a/tests/test_composite_action_coverage.py b/tests/test_composite_action_coverage.py index 1659039..5de628a 100644 --- a/tests/test_composite_action_coverage.py +++ b/tests/test_composite_action_coverage.py @@ -239,7 +239,7 @@ def test_workflow_dispatch_pr_number_zero( self, action_tester, default_inputs, default_github_context ): """Test workflow_dispatch with PR_NUMBER=0 enables bulk mode.""" - env_vars = {} + env_vars: dict[str, str] = {} inputs = default_inputs.copy() inputs["PR_NUMBER"] = "0" github_context = default_github_context.copy() @@ -261,7 +261,7 @@ def test_workflow_dispatch_specific_pr_number( self, action_tester, default_inputs, default_github_context ): """Test workflow_dispatch with specific PR number.""" - env_vars = {} + env_vars: dict[str, str] = {} inputs = default_inputs.copy() inputs["PR_NUMBER"] = "42" github_context = default_github_context.copy() @@ -282,7 +282,7 @@ def test_workflow_dispatch_invalid_pr_number( self, action_tester, default_inputs, default_github_context ): """Test workflow_dispatch with invalid PR number.""" - env_vars = {} + env_vars: dict[str, str] = {} inputs = default_inputs.copy() inputs["PR_NUMBER"] = "abc" github_context = default_github_context.copy() @@ -302,7 +302,7 @@ def test_non_dispatch_rejects_explicit_pr_number( self, action_tester, default_inputs, default_github_context ): """Test that non-dispatch events reject explicit PR_NUMBER.""" - env_vars = {} + env_vars: dict[str, str] = {} inputs = default_inputs.copy() inputs["PR_NUMBER"] = "42" github_context = default_github_context.copy() @@ -580,7 +580,7 @@ def test_validation_steps_exit_codes( ): """Test that validation steps return appropriate exit codes.""" # Test invalid PR number scenario - env_vars = {} + env_vars: dict[str, str] = {} inputs = default_inputs.copy() inputs["PR_NUMBER"] = "invalid" github_context = default_github_context.copy() 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_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_error_codes.py b/tests/test_error_codes.py index c0ecc34..2d08767 100644 --- a/tests/test_error_codes.py +++ b/tests/test_error_codes.py @@ -29,16 +29,27 @@ class TestExitCode: def test_exit_codes_are_integers(self): """Test that exit codes are valid integers.""" - assert ExitCode.SUCCESS == 0 - assert ExitCode.GENERAL_ERROR == 1 - assert ExitCode.CONFIGURATION_ERROR == 2 - assert ExitCode.DUPLICATE_ERROR == 3 - assert ExitCode.GITHUB_API_ERROR == 4 - assert ExitCode.GERRIT_CONNECTION_ERROR == 5 - assert ExitCode.NETWORK_ERROR == 6 - assert ExitCode.REPOSITORY_ERROR == 7 - assert ExitCode.PR_STATE_ERROR == 8 - assert ExitCode.VALIDATION_ERROR == 9 + # Driven from a list rather than written as ten literal + # assertions. ExitCode is an IntEnum, so every comparison below + # holds at runtime, but mypy narrows a bare ``ExitCode.SUCCESS`` + # to Literal[ExitCode.SUCCESS] and then reports comparison-overlap + # against Literal[0]. Binding each member to an ExitCode-typed + # variable keeps the identical ``==`` checks while leaving mypy + # the declared type, which does overlap with int. + expected: list[tuple[ExitCode, int]] = [ + (ExitCode.SUCCESS, 0), + (ExitCode.GENERAL_ERROR, 1), + (ExitCode.CONFIGURATION_ERROR, 2), + (ExitCode.DUPLICATE_ERROR, 3), + (ExitCode.GITHUB_API_ERROR, 4), + (ExitCode.GERRIT_CONNECTION_ERROR, 5), + (ExitCode.NETWORK_ERROR, 6), + (ExitCode.REPOSITORY_ERROR, 7), + (ExitCode.PR_STATE_ERROR, 8), + (ExitCode.VALIDATION_ERROR, 9), + ] + for code, value in expected: + assert code == value def test_all_exit_codes_have_messages(self): """Test that all exit codes have corresponding error messages.""" diff --git a/tests/test_gerrit_ssh_abandon.py b/tests/test_gerrit_ssh_abandon.py index 3b7e4d7..a24ef87 100644 --- a/tests/test_gerrit_ssh_abandon.py +++ b/tests/test_gerrit_ssh_abandon.py @@ -67,9 +67,8 @@ def test_returns_false_when_prerequisites_missing(self): def test_mkdtemp_failure_returns_false(self): # Honor the "never raises" contract: a temp-dir failure must result # in a clean False so the caller can fall back to REST. - with patch.object( - gerrit_ssh.tempfile, - "mkdtemp", + with patch( + "github2gerrit.gerrit_ssh.tempfile.mkdtemp", side_effect=OSError("disk full"), ): result = gerrit_ssh.abandon_change_via_ssh( 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_digest_and_backref.py b/tests/test_mapping_comment_digest_and_backref.py index e4ea974..92df0b2 100644 --- a/tests/test_mapping_comment_digest_and_backref.py +++ b/tests/test_mapping_comment_digest_and_backref.py @@ -150,7 +150,7 @@ def _fake_run_cmd(cmd, cwd=None, env=None): ) # Invoke back-reference logic - orch._add_backref_comment_in_gerrit( # type: ignore[attr-defined] + orch._add_backref_comment_in_gerrit( gerrit=gerrit, repo=repo_names, branch="main", diff --git a/tests/test_misc_small_coverage.py b/tests/test_misc_small_coverage.py index a7726aa..8bc5779 100644 --- a/tests/test_misc_small_coverage.py +++ b/tests/test_misc_small_coverage.py @@ -37,7 +37,7 @@ def _capture_logger() -> tuple[logging.Logger, io.StringIO]: return logger, stream -@pytest.mark.parametrize("verbose_env", ["", "false", "true"]) # type: ignore[misc] +@pytest.mark.parametrize("verbose_env", ["", "false", "true"]) def test_log_exception_conditionally_toggle( monkeypatch: pytest.MonkeyPatch, verbose_env: str ) -> None: diff --git a/tests/test_reconciliation_plan_and_orphans.py b/tests/test_reconciliation_plan_and_orphans.py index 6fcf770..2a00f1a 100644 --- a/tests/test_reconciliation_plan_and_orphans.py +++ b/tests/test_reconciliation_plan_and_orphans.py @@ -119,7 +119,8 @@ def _extract_summary(caplog) -> dict[str, Any]: # Expect pattern: RECONCILE_SUMMARY json={...} m = re.search(r"RECONCILE_SUMMARY json=(\{.*\})", last) assert m, f"Could not parse summary JSON: {last}" - return json.loads(m.group(1)) + summary: dict[str, Any] = json.loads(m.group(1)) + return summary def _extract_orphan_actions(caplog) -> dict[str, Any] | None: @@ -129,7 +130,8 @@ def _extract_orphan_actions(caplog) -> dict[str, Any] | None: m = re.search(r"ORPHAN_ACTIONS json=(\{.*\})", lines[-1]) if not m: return None - return json.loads(m.group(1)) + actions: dict[str, Any] = json.loads(m.group(1)) + return actions # --------------------------------------------------------------------------- diff --git a/tests/test_ssh_agent_ownership.py b/tests/test_ssh_agent_ownership.py index b3f678d..2fe5fb8 100644 --- a/tests/test_ssh_agent_ownership.py +++ b/tests/test_ssh_agent_ownership.py @@ -14,6 +14,27 @@ from github2gerrit.ssh_agent_setup import SSHAgentManager +def _simulate_agent_state( + manager: SSHAgentManager, + *, + pid: int | None, + sock: str | None, + owned: bool, +) -> None: + """Put *manager* into a known agent state before exercising cleanup. + + Setting the attributes through declared-Optional parameters rather + than inline in the test body also keeps mypy from drawing the wrong + conclusion: mypy narrows ``manager.agent_pid`` to ``int`` on a direct + ``manager.agent_pid = 12345`` and never widens it again, not even + across the ``cleanup()`` call that resets it, so the following + ``assert manager.agent_pid is None`` is reported as unreachable. + """ + manager.agent_pid = pid + manager.auth_sock = sock + manager._agent_owned_by_us = owned + + class TestSSHAgentOwnership: """Test SSH agent ownership tracking to prevent killing borrowed agents.""" @@ -134,9 +155,12 @@ def test_cleanup_kills_owned_agent(self, mock_run_cmd): manager = SSHAgentManager(workspace=Path(tmp_dir)) # Simulate owned agent - manager.agent_pid = 12345 - manager.auth_sock = f"{tmp_dir}/ssh-agent.sock" - manager._agent_owned_by_us = True + _simulate_agent_state( + manager, + pid=12345, + sock=f"{tmp_dir}/ssh-agent.sock", + owned=True, + ) manager.cleanup() @@ -157,9 +181,12 @@ def test_cleanup_does_not_kill_borrowed_agent(self, mock_run_cmd): manager = SSHAgentManager(workspace=Path(tmp_dir)) # Simulate borrowed agent - manager.agent_pid = 54321 - manager.auth_sock = f"{tmp_dir}/existing-agent.sock" - manager._agent_owned_by_us = False + _simulate_agent_state( + manager, + pid=54321, + sock=f"{tmp_dir}/existing-agent.sock", + owned=False, + ) manager.cleanup() @@ -178,9 +205,12 @@ def test_cleanup_handles_kill_failure_gracefully(self, mock_run_cmd): manager = SSHAgentManager(workspace=Path(tmp_dir)) # Simulate owned agent - manager.agent_pid = 12345 - manager.auth_sock = f"{tmp_dir}/ssh-agent.sock" - manager._agent_owned_by_us = True + _simulate_agent_state( + manager, + pid=12345, + sock=f"{tmp_dir}/ssh-agent.sock", + owned=True, + ) # Mock kill failure mock_run_cmd.side_effect = Exception("Process not found") diff --git a/tests/unit/test_config_integration.py b/tests/unit/test_config_integration.py index 8287eb1..6d44ca1 100644 --- a/tests/unit/test_config_integration.py +++ b/tests/unit/test_config_integration.py @@ -18,16 +18,11 @@ from github2gerrit.config import apply_parameter_derivation from github2gerrit.config import derive_gerrit_parameters -from github2gerrit.ssh_config_parser import clear_credential_cache class TestDeriveGerritParameters: """Test cases for derive_gerrit_parameters function.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_no_organization(self): """Test behavior when no organization is provided.""" result = derive_gerrit_parameters(None) @@ -163,10 +158,6 @@ def test_case_normalization(self): class TestApplyParameterDerivation: """Test cases for apply_parameter_derivation function.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_no_organization(self): """Test that no derivation occurs without organization.""" cfg = {"EXISTING_KEY": "existing_value"} @@ -182,7 +173,7 @@ def test_derivation_disabled(self, mock_is_actions, mock_derive): mock_is_actions.return_value = False with mock.patch.dict(os.environ, {"G2G_ENABLE_DERIVATION": "false"}): - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation(cfg, "testorg") assert result == cfg @@ -198,7 +189,7 @@ def test_derivation_enabled_by_default(self, mock_is_actions, mock_derive): "GERRIT_SSH_USER_G2G_EMAIL": "derived@example.com", } - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation( cfg, "testorg", save_to_config=False ) @@ -264,10 +255,6 @@ def test_empty_values_are_derived(self, mock_is_actions, mock_derive): class TestConfigIntegration: """Integration tests for configuration with SSH config parsing.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_real_world_lfit_scenario(self, tmp_path): """Test real-world scenario with lfit organization configuration.""" # Create a temporary SSH config @@ -315,7 +302,7 @@ def test_real_world_lfit_scenario(self, tmp_path): mock_git.return_value = "user@example.org" # Test the full integration - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation( cfg, "exampleorg", save_to_config=False ) @@ -346,7 +333,7 @@ def test_fallback_for_unknown_org(self, tmp_path): ) as mock_git: mock_git.return_value = None - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation( cfg, "unknownorg", save_to_config=False ) @@ -403,7 +390,7 @@ def test_mixed_configuration_sources(self, tmp_path): ) as mock_git: mock_git.return_value = "user@custom.org" - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation( cfg, "testorg", save_to_config=False ) @@ -455,7 +442,7 @@ def test_organization_config_affects_ssh_lookup( } mock_derive.return_value = ("customuser", "custom@example.com") - cfg = {} + cfg: dict[str, str] = {} result = apply_parameter_derivation( cfg, "testorg", save_to_config=False ) diff --git a/tests/unit/test_ssh_config_parser.py b/tests/unit/test_ssh_config_parser.py index f4b8c89..260afaf 100644 --- a/tests/unit/test_ssh_config_parser.py +++ b/tests/unit/test_ssh_config_parser.py @@ -15,7 +15,6 @@ import pytest from github2gerrit.ssh_config_parser import SSHConfig -from github2gerrit.ssh_config_parser import clear_credential_cache from github2gerrit.ssh_config_parser import derive_gerrit_credentials from github2gerrit.ssh_config_parser import get_git_user_email from github2gerrit.ssh_config_parser import get_ssh_user_for_gerrit @@ -24,10 +23,6 @@ class TestSSHConfig: """Test cases for SSH configuration parsing.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_empty_config(self, tmp_path): """Test handling of empty SSH config file.""" config_file = tmp_path / "config" @@ -303,10 +298,6 @@ def test_case_insensitive_directives(self, tmp_path): class TestConvenienceFunctions: """Test cases for convenience functions.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_get_ssh_user_for_gerrit(self, tmp_path): """Test the convenience function for getting Gerrit SSH user.""" config_content = """ @@ -498,10 +489,6 @@ def test_validate_git_executable_timeout(self, mock_run): class TestCredentialDerivation: """Test cases for credential derivation logic.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - @mock.patch("github2gerrit.ssh_config_parser.get_git_user_email") @mock.patch("github2gerrit.ssh_config_parser.get_ssh_user_for_gerrit") @mock.patch.dict("os.environ", {"G2G_RESPECT_USER_SSH": "true"}) @@ -591,10 +578,6 @@ def test_derive_gerrit_credentials_custom_port(self): class TestPatternMatching: """Test cases for SSH host pattern matching.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_pattern_matching_exact(self): """Test exact host pattern matching.""" ssh_config = SSHConfig() @@ -691,10 +674,6 @@ def test_pattern_matching_global_wildcard(self): class TestConfigLineHandling: """Test cases for SSH config line parsing.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - def test_split_config_line_basic(self): """Test basic config line splitting.""" ssh_config = SSHConfig() @@ -768,10 +747,6 @@ def sample_ssh_config(): class TestIntegration: """Integration tests using realistic configurations.""" - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() - @mock.patch.dict("os.environ", {"G2G_RESPECT_USER_SSH": "true"}) def test_lfit_organization_config(self, tmp_path, sample_ssh_config): """Test credential derivation for lfit organization with real SSH config.""" @@ -829,19 +804,24 @@ def test_unknown_organization_fallback(self, tmp_path, sample_ssh_config): class TestCaching: - """Test cases for caching behavior in SSH config parsing and credential derivation.""" + """Test cases proving credential derivation caches nothing. - def setup_method(self): - """Clear caches before each test to ensure isolation.""" - clear_credential_cache() + The module holds no process-wide state, so each derivation and each + reading of ``G2G_RESPECT_USER_SSH`` consults its source afresh. + """ @mock.patch("github2gerrit.ssh_config_parser.get_git_user_email") @mock.patch("github2gerrit.ssh_config_parser.get_ssh_user_for_gerrit") @mock.patch("github2gerrit.ssh_config_parser._get_respect_user_ssh_setting") - def test_derive_gerrit_credentials_caching( + def test_derive_gerrit_credentials_rereads_its_sources( self, mock_respect_ssh, mock_ssh_user, mock_git_email ): - """Test that derive_gerrit_credentials caches results to avoid repeated calls.""" + """Repeated derivation re-reads its sources rather than memoising. + + The result depends on the SSH config and the git identity, and + neither is an argument, so a memo keyed on the arguments could + serve an answer computed from state that has since changed. + """ # Set up mocks to enable SSH config usage mock_respect_ssh.return_value = True mock_ssh_user.return_value = "sshuser" @@ -859,17 +839,15 @@ def test_derive_gerrit_credentials_caching( assert user1 == user2 == "sshuser" assert email1 == email2 == "user@example.com" - # The underlying functions should only be called once due to caching - # Note: mock_respect_ssh will be called twice because it's also cached separately - assert mock_ssh_user.call_count == 1 - assert mock_git_email.call_count == 1 + # Both underlying sources are consulted on both calls + assert mock_ssh_user.call_count == 2 + assert mock_git_email.call_count == 2 @mock.patch("github2gerrit.ssh_config_parser.env_bool") - def test_respect_user_ssh_setting_caching(self, mock_env_bool): - """Test that G2G_RESPECT_USER_SSH environment variable is cached.""" + def test_respect_user_ssh_setting_reads_env_every_call(self, mock_env_bool): + """G2G_RESPECT_USER_SSH is read on every call.""" mock_env_bool.return_value = True - # Import the cached function from github2gerrit.ssh_config_parser import ( _get_respect_user_ssh_setting, ) @@ -882,41 +860,150 @@ def test_respect_user_ssh_setting_caching(self, mock_env_bool): # All should return the same value assert result1 == result2 == result3 is True - # env_bool should only be called once due to caching - assert mock_env_bool.call_count == 1 + # The environment is consulted afresh on each call, so that a + # later assignment to the variable is not masked by a memo. + assert mock_env_bool.call_count == 3 - def test_clear_credential_cache_functionality(self): - """Test that clear_credential_cache properly clears all caches.""" - # Import the cache clearing function and cached functions - from github2gerrit.ssh_config_parser import _get_cached_git_user_email - from github2gerrit.ssh_config_parser import ( - _get_respect_user_ssh_setting, - ) - from github2gerrit.ssh_config_parser import clear_credential_cache - # Call functions to populate caches (using mocks to avoid actual system calls) - with mock.patch( - "github2gerrit.ssh_config_parser.env_bool" - ) as mock_env_bool: - mock_env_bool.return_value = True - _get_respect_user_ssh_setting() +_HOST = "gerrit.example.com" +_ORG = "testorg" +_FALLBACK = ( + "testorg.gh2gerrit", + "releng+testorg-gh2gerrit@linuxfoundation.org", +) - with mock.patch( - "github2gerrit.ssh_config_parser.get_git_user_email" - ) as mock_git_email: - mock_git_email.return_value = "test@example.com" - _get_cached_git_user_email() - with mock.patch( - "github2gerrit.ssh_config_parser.get_ssh_user_for_gerrit" - ) as mock_ssh_user: - mock_ssh_user.return_value = "testuser" - derive_gerrit_credentials("gerrit.example.com", "testorg") - - # Clear all caches - clear_credential_cache() - - # Verify caches are cleared by checking cache info - assert _get_respect_user_ssh_setting.cache_info().currsize == 0 - assert _get_cached_git_user_email.cache_info().currsize == 0 - assert derive_gerrit_credentials.cache_info().currsize == 0 +class TestDerivationReflectsItsInputs: + """Regression tests for issue #394. + + ``derive_gerrit_credentials`` reads three things that are not among + its arguments: ``G2G_RESPECT_USER_SSH``, ``~/.ssh/config`` and the + git identity. Each test below changes one of them and asserts the + derived credentials change with it. + + These tests deliberately clear nothing between the two derivations + within a test. That is the property under test: derivation is sound + because it re-reads its inputs, not because something sweeps up + after it. + """ + + def test_respect_user_ssh_flag_change_switches_source( + self, monkeypatch: pytest.MonkeyPatch + ): + """Flipping the flag switches between SSH identity and fallback. + + Same host, organisation and port for both derivations, so a memo + keyed on those would return the personal SSH identity for the + second call -- the exact wrong-credentials failure of #394. + """ + with ( + mock.patch( + "github2gerrit.ssh_config_parser.get_ssh_user_for_gerrit", + return_value="sshuser", + ), + mock.patch( + "github2gerrit.ssh_config_parser.get_git_user_email", + return_value="dev@example.org", + ), + ): + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + respected = derive_gerrit_credentials(_HOST, _ORG) + + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "false") + ignored = derive_gerrit_credentials(_HOST, _ORG) + + assert respected == ("sshuser", "dev@example.org") + assert ignored == _FALLBACK + + def test_git_email_change_changes_derived_email( + self, monkeypatch: pytest.MonkeyPatch + ): + """A change of git user.email reaches the derived credentials.""" + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + emails = iter(["first@example.org", "second@example.org"]) + + with ( + mock.patch( + "github2gerrit.ssh_config_parser.get_ssh_user_for_gerrit", + return_value="sshuser", + ), + mock.patch( + "github2gerrit.ssh_config_parser.get_git_user_email", + side_effect=lambda: next(emails), + ), + ): + first = derive_gerrit_credentials(_HOST, _ORG) + second = derive_gerrit_credentials(_HOST, _ORG) + + assert first == ("sshuser", "first@example.org") + assert second == ("sshuser", "second@example.org") + + def test_ssh_config_rewrite_changes_derived_user( + self, monkeypatch: pytest.MonkeyPatch, tmp_path + ): + """Rewriting ~/.ssh/config under an unchanged HOME is noticed. + + ``get_ssh_user_for_gerrit`` is left unmocked so the real parse + is exercised. Nothing about the rewrite is visible in the + path, which is all the second call has to go on before it opens + the file, so the new user name can only reach the caller if the + file is read again. + """ + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + ssh_dir = tmp_path / "home" / ".ssh" + ssh_dir.mkdir(parents=True) + config_file = ssh_dir / "config" + + with ( + mock.patch( + "github2gerrit.ssh_config_parser.Path.home" + ) as mock_home, + mock.patch( + "github2gerrit.ssh_config_parser.get_git_user_email", + return_value="dev@example.org", + ), + ): + mock_home.return_value = tmp_path / "home" + + config_file.write_text(f"Host {_HOST}\n User firstuser\n") + first = derive_gerrit_credentials(_HOST, _ORG) + + config_file.write_text(f"Host {_HOST}\n User second_user\n") + second = derive_gerrit_credentials(_HOST, _ORG) + + assert first == ("firstuser", "dev@example.org") + assert second == ("second_user", "dev@example.org") + + def test_ssh_config_appearing_later_is_picked_up( + self, monkeypatch: pytest.MonkeyPatch, tmp_path + ): + """A config file created after a first miss is still parsed. + + A missing file is an ordinary state, not an error, so the first + derivation falls back silently. That silence must not become + permanent: the second call has to look for the file again + rather than assume the first answer still holds. + """ + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + ssh_dir = tmp_path / "home" / ".ssh" + ssh_dir.mkdir(parents=True) + config_file = ssh_dir / "config" + + with ( + mock.patch( + "github2gerrit.ssh_config_parser.Path.home" + ) as mock_home, + mock.patch( + "github2gerrit.ssh_config_parser.get_git_user_email", + return_value="dev@example.org", + ), + ): + mock_home.return_value = tmp_path / "home" + + missing = derive_gerrit_credentials(_HOST, _ORG) + + config_file.write_text(f"Host {_HOST}\n User lateuser\n") + present = derive_gerrit_credentials(_HOST, _ORG) + + assert missing == (_FALLBACK[0], "dev@example.org") + assert present == ("lateuser", "dev@example.org") diff --git a/uv.lock b/uv.lock index 3f18d56..bf47bf8 100644 --- a/uv.lock +++ b/uv.lock @@ -403,6 +403,7 @@ dev = [ { name = "pytest-mock" }, { name = "responses" }, { name = "ruff" }, + { name = "types-pyyaml" }, { name = "types-requests" }, { name = "types-urllib3" }, ] @@ -433,6 +434,7 @@ requires-dist = [ { name = "rich", specifier = ">=14.2.0" }, { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.6.3" }, { name = "typer", specifier = ">=0.20.1" }, + { name = "types-pyyaml", marker = "extra == 'dev'", specifier = ">=6.0.12" }, { name = "types-requests", marker = "extra == 'dev'", specifier = ">=2.31.0" }, { name = "types-urllib3", marker = "extra == 'dev'", specifier = ">=1.26.25.14" }, { name = "urllib3", specifier = ">=2.6.3" }, @@ -1067,6 +1069,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/43/89/9518bc0c3929bee36b3a4a8e3daddd6e03f92f9961c66d4983b837160543/typer-0.27.1-py3-none-any.whl", hash = "sha256:53150287edd11baeb4e4722c8e394fcdf8181c0ae89485cba8d25c778d5edd56", size = 122874, upload-time = "2026-08-03T14:41:04.391Z" }, ] +[[package]] +name = "types-pyyaml" +version = "6.0.12.20260815" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/9f/72/b56089aeee6c496d969bac42376bedb6e3eeab4682e1018fa3137122f94b/types_pyyaml-6.0.12.20260815.tar.gz", hash = "sha256:28764110c9cf35846e733da32d8d734df7473c5dde9ef67c3b7332ec0e819858", size = 18545, upload-time = "2026-08-15T02:41:51.532Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/08/52/eefeba09be4ef2a1eb989eb92934561e8e502a6ee3c32654996e4be7e399/types_pyyaml-6.0.12.20260815-py3-none-any.whl", hash = "sha256:6f332212b7e191f3afd5016a713c510b6340593b7ebec573c7d5d20aa5386d3b", size = 21148, upload-time = "2026-08-15T02:41:50.555Z" }, +] + [[package]] name = "types-requests" version = "2.33.0.20260712"