From c2f64e3e557aedb87a684953d88be712973227b9 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 10 Jun 2026 14:28:27 +0200 Subject: [PATCH 1/9] Abstract bump lockfile operations behind a repository port BumpOptions, a public domain dataclass, exposed a CommandRunner field, leaking execution infrastructure into the domain boundary. Introduce the LockfileRepository protocol in bump_lockfiles as the port through which the bump domain projects and regenerates Cargo lockfiles, with CargoLockfileRepository as the cargo-backed adapter bound to a command runner. BumpOptions now carries lockfile_repository instead of command_runner; the CLI binds the adapter to its selected runner at the composition root, and bump falls back to the default adapter when no repository is injected. The port's documented scope is bump-side lockfile projection and regeneration; publish-side discovery and validation continue to take a CommandRunner directly. Tests inject a recording repository to verify live runs regenerate and dry runs only project, without touching Cargo. Closes #82 --- docs/developers-guide.md | 15 ++++-- lading/cli.py | 4 +- lading/commands/__init__.py | 4 +- lading/commands/bump.py | 27 +++++----- lading/commands/bump_lockfiles.py | 48 +++++++++++++++++ tests/unit/test_bump_lockfile_rebuild.py | 67 ++++++++++++++++++++++++ tests/unit/test_cli.py | 5 +- 7 files changed, 147 insertions(+), 23 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index ffa2eff6..5253f09b 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -210,11 +210,16 @@ only the workspace-root lockfile is processed, its lone failure is re-raised as the original cargo error rather than wrapped in the aggregate message. `BumpOptions` carries the dependency-injection points used by -`lading.commands.bump.run`. The `command_runner` field accepts an optional -`CommandRunner`, matching the command-runner protocol used by publish -execution. When `command_runner` is `None`, bump falls back to the default -subprocess runner. Tests pass a runner explicitly so lockfile commands can be -observed without invoking real Cargo processes. +`lading.commands.bump.run`. Lockfile operations are reached through the +`lockfile_repository` field, a `bump_lockfiles.LockfileRepository` port +introduced by issue 82: the bump domain never holds a raw command runner. When +the field is `None`, bump uses `bump_lockfiles.CargoLockfileRepository`, the +cargo-backed adapter bound to the default subprocess runner; the CLI binds the +adapter to its selected runner. Tests inject a repository (or bind the adapter +to a recording runner) so lockfile commands can be observed without invoking +real Cargo processes. The port's scope is bump-side lockfile projection and +regeneration only; publish-side discovery and validation continue to take a +`CommandRunner` directly in `lockfile.py`. Bump-time crate-set derivation is centralized in the bump context: the `excluded` and `updated_crate_names` sets are computed exactly once in diff --git a/lading/cli.py b/lading/cli.py index 98398d1b..a1017258 100644 --- a/lading/cli.py +++ b/lading/cli.py @@ -341,7 +341,9 @@ def bump( rebuild_lockfiles=rebuild_lockfiles, configuration=configuration, workspace=workspace, - command_runner=command_runner, + lockfile_repository=commands.bump_lockfiles.CargoLockfileRepository( + runner=command_runner + ), ), ), ) diff --git a/lading/commands/__init__.py b/lading/commands/__init__.py index 31c21733..ae11b807 100644 --- a/lading/commands/__init__.py +++ b/lading/commands/__init__.py @@ -2,6 +2,6 @@ from __future__ import annotations -from . import bump, publish +from . import bump, bump_lockfiles, publish -__all__ = ["bump", "publish"] +__all__ = ["bump", "bump_lockfiles", "publish"] diff --git a/lading/commands/bump.py b/lading/commands/bump.py index 95800a95..eed78ef6 100644 --- a/lading/commands/bump.py +++ b/lading/commands/bump.py @@ -61,7 +61,6 @@ from lading.commands.bump_manifests import _BumpContext from lading.config import LadingConfig - from lading.runtime import CommandRunner from lading.workspace import WorkspaceCrate, WorkspaceGraph LOGGER = logging.getLogger(__name__) @@ -88,9 +87,11 @@ class BumpOptions: workspace : WorkspaceGraph | None, default None Loaded workspace graph. Programmatic callers may omit it only when they want ``run`` to inspect the workspace. - command_runner : CommandRunner | None, default None - Callable with the runtime command-runner interface. It is used for - lockfile rebuild commands; ``None`` uses the default subprocess runner. + lockfile_repository : bump_lockfiles.LockfileRepository | None, default None + Port used for lockfile projection and regeneration. ``None`` selects + the cargo-backed adapter with the default subprocess runner. Tests + inject a repository so lockfile behaviour can be observed without + invoking real Cargo processes. dependency_sections : Mapping[str, Collection[str]] Explicit dependency sections to rewrite by crate name. include_workspace_sections : bool, default False @@ -106,7 +107,7 @@ class BumpOptions: rebuild_lockfiles: bool | None = None configuration: LadingConfig | None = None workspace: WorkspaceGraph | None = None - command_runner: CommandRunner | None = None + lockfile_repository: bump_lockfiles.LockfileRepository | None = None dependency_sections: cabc.Mapping[str, cabc.Collection[str]] = dc.field( default_factory=lambda: types.MappingProxyType({}) ) @@ -192,7 +193,7 @@ def _initialize_bump_context( rebuild_lockfiles=rebuild_lockfiles, configuration=configuration, workspace=workspace, - command_runner=resolved_options.command_runner, + lockfile_repository=resolved_options.lockfile_repository, dependency_sections=resolved_options.dependency_sections, include_workspace_sections=resolved_options.include_workspace_sections, ) @@ -319,15 +320,13 @@ def _process_lockfiles( if context.base_options.rebuild_lockfiles is not True or not changed_manifests: return () lockfile_manifests = context.configuration.bump.lockfile_manifests - if context.base_options.dry_run: - return bump_lockfiles.resolve_lockfile_paths( - context.root_path, lockfile_manifests - ) - return bump_lockfiles.regenerate_lockfiles( - context.root_path, - lockfile_manifests, - runner=context.base_options.command_runner, + repository = ( + context.base_options.lockfile_repository + or bump_lockfiles.CargoLockfileRepository() ) + if context.base_options.dry_run: + return repository.resolve_lockfile_paths(context.root_path, lockfile_manifests) + return repository.regenerate_lockfiles(context.root_path, lockfile_manifests) def _prepare_sorted_changes( diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index 7109ae97..26a82ef2 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -237,3 +237,51 @@ def _run_workspace_lockfile_update( stderr, ) raise LockfileRegenerationError(message) + + +@dc.dataclass(frozen=True, slots=True) +class CargoLockfileRepository: + """Cargo-backed :class:`LockfileRepository` bound to a command runner.""" + + runner: CommandRunner | None = None + + def resolve_lockfile_paths( + self, + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[Path, ...]: + """Return the lockfile paths a regeneration run would touch.""" + return resolve_lockfile_paths(workspace_root, lockfile_manifests) + + def regenerate_lockfiles( + self, + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[Path, ...]: + """Regenerate lockfiles via ``cargo update --workspace``.""" + return regenerate_lockfiles( + workspace_root, lockfile_manifests, runner=self.runner + ) + + +class LockfileRepository(typ.Protocol): + """Port for projecting and regenerating Cargo lockfiles after a bump. + + The bump domain depends on this protocol rather than on a command + runner, keeping execution infrastructure out of the public bump options + (issue #82). + """ + + def resolve_lockfile_paths( + self, + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[Path, ...]: + """Return the lockfile paths a regeneration run would touch.""" + + def regenerate_lockfiles( + self, + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[Path, ...]: + """Regenerate lockfiles and return the rewritten paths.""" diff --git a/tests/unit/test_bump_lockfile_rebuild.py b/tests/unit/test_bump_lockfile_rebuild.py index b4445a8d..1f5543b5 100644 --- a/tests/unit/test_bump_lockfile_rebuild.py +++ b/tests/unit/test_bump_lockfile_rebuild.py @@ -2,6 +2,7 @@ from __future__ import annotations +import collections.abc as cabc import dataclasses as dc import pathlib import typing as typ @@ -186,6 +187,72 @@ def fail_regeneration(*args: object, **kwargs: object) -> typ.NoReturn: assert message == snapshot +class _RecordingLockfileRepository: + """LockfileRepository double recording calls without touching Cargo.""" + + def __init__(self) -> None: + self.resolved: list[tuple[pathlib.Path, tuple[str, ...]]] = [] + self.regenerated: list[tuple[pathlib.Path, tuple[str, ...]]] = [] + + def resolve_lockfile_paths( + self, + workspace_root: pathlib.Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[pathlib.Path, ...]: + self.resolved.append((workspace_root, tuple(lockfile_manifests))) + return (workspace_root / "Cargo.lock",) + + def regenerate_lockfiles( + self, + workspace_root: pathlib.Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[pathlib.Path, ...]: + self.regenerated.append((workspace_root, tuple(lockfile_manifests))) + return (workspace_root / "Cargo.lock",) + + +def test_run_uses_injected_lockfile_repository(tmp_path: pathlib.Path) -> None: + """Bump reaches lockfile operations only through the repository port.""" + workspace = _make_workspace(tmp_path) + repository = _RecordingLockfileRepository() + + message = bump.run( + tmp_path, + "1.2.3", + options=bump.BumpOptions( + configuration=_make_config(), + workspace=workspace, + lockfile_repository=repository, + ), + ) + + assert repository.regenerated == [(tmp_path.resolve(), ())] + assert repository.resolved == [] + assert "Cargo.lock (lockfile)" in message + + +def test_dry_run_projects_through_lockfile_repository( + tmp_path: pathlib.Path, +) -> None: + """Dry runs project lockfile paths without regenerating.""" + workspace = _make_workspace(tmp_path) + repository = _RecordingLockfileRepository() + + bump.run( + tmp_path, + "1.2.3", + options=bump.BumpOptions( + dry_run=True, + configuration=_make_config(), + workspace=workspace, + lockfile_repository=repository, + ), + ) + + assert repository.resolved == [(tmp_path.resolve(), ())] + assert repository.regenerated == [] + + @pytest.mark.parametrize( "scenario", [ diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index af253b3e..ad5e427d 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -21,6 +21,7 @@ from lading import cli from lading import config as config_module from lading.commands import bump as bump_command +from lading.commands import bump_lockfiles from lading.commands import publish as publish_command from lading.utils import normalise_workspace_root from lading.workspace import WorkspaceCrate, WorkspaceGraph @@ -398,7 +399,9 @@ def fake_run(*args: object, **kwargs: object) -> str: options = captured_kwargs["options"] assert isinstance(options, bump_command.BumpOptions) assert options.dry_run is True - assert options.command_runner is cli.subprocess_runner + repository = options.lockfile_repository + assert isinstance(repository, bump_lockfiles.CargoLockfileRepository) + assert repository.runner is cli.subprocess_runner def test_publish_cli_logs_dry_run_default_flag_resolution( From 5624e6a38e8b28ca092dcd000bdba3fd295aebbd Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 23:14:26 +0200 Subject: [PATCH 2/9] Route publish-side lockfile inspection through a repository port Extend the issue #82 repository/VCS port abstraction to the publish side. Previously `lockfile.py` discovery and freshness validation, and the `publish_preflight` domain that drives them, took a `CommandRunner` directly, mixing git VCS, filesystem, and cargo execution with the freshness-classification logic. Add a `LockfileInspectionRepository` port and a git/cargo-backed `CargoLockfileInspectionRepository` adapter (binding a runner and the optional pre-flight environment). `_validate_lockfile_freshness` and `_collect_stale_lockfiles` now depend only on the port; `_run_preflight_checks` is the composition root that binds the adapter, and tests inject a port double at the `_validate_lockfile_freshness` seam. This is the publish-side counterpart to the bump-side `bump_lockfiles.LockfileRepository`, so neither lockfile domain holds a raw command runner. Rewrite the pre-flight validation tests to inject a recording port double and add adapter tests covering env binding, delegation, and the `manifest_exists` predicate. Update the developer guide to document the publish-side port and drop the bump-side-only scope caveat. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/developers-guide.md | 24 ++- lading/commands/lockfile.py | 78 +++++++- lading/commands/publish_preflight.py | 52 +++--- .../test_preflight_lockfile_validation.py | 169 +++++++----------- tests/unit/test_lockfile.py | 90 ++++++++++ 5 files changed, 277 insertions(+), 136 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 5253f09b..9b346d04 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -218,8 +218,10 @@ cargo-backed adapter bound to the default subprocess runner; the CLI binds the adapter to its selected runner. Tests inject a repository (or bind the adapter to a recording runner) so lockfile commands can be observed without invoking real Cargo processes. The port's scope is bump-side lockfile projection and -regeneration only; publish-side discovery and validation continue to take a -`CommandRunner` directly in `lockfile.py`. +regeneration; publish-side discovery and validation go through the sibling +`lockfile.LockfileInspectionRepository` port (see the Lockfile helpers section +below), so neither the bump nor the publish lockfile domain holds a raw +`CommandRunner` (issue 82). Bump-time crate-set derivation is centralized in the bump context: the `excluded` and `updated_crate_names` sets are computed exactly once in @@ -335,8 +337,22 @@ manifest rewrites, while publish only probes freshness read-only via `cargo metadata --locked --manifest-path ... --format-version=1`. It returns a `LockfileFreshness` result that distinguishes fresh lockfiles, lockfiles that Cargo says need updating under `--locked`, and unrelated Cargo failures. -`_validate_lockfile_freshness` in `publish_preflight.py` calls it before the -cargo check/test pre-flight. + +The publish pre-flight domain reaches both operations through the +`LockfileInspectionRepository` port (issue 82) rather than holding a command +runner: `_validate_lockfile_freshness` and `_collect_stale_lockfiles` in +`publish_preflight.py` depend only on the port, so VCS (git), filesystem, and +cargo execution concerns stay out of the freshness-classification logic. +`CargoLockfileInspectionRepository` is the git- and cargo-backed adapter; it +binds a `CommandRunner` and the optional pre-flight base environment, applying +that environment to any invocation that does not supply its own. +`publish_preflight._run_preflight_checks` is the composition root: it binds the +adapter to the selected command runner and pre-flight environment. Tests inject +a port double at the `_validate_lockfile_freshness` seam, observing discovery +and validation without invoking real git or cargo. This is the publish-side +counterpart to the bump-side `bump_lockfiles.LockfileRepository`; together they +complete issue 82's separation of lockfile VCS/filesystem concerns from the +command domain. `LockfileDiscoveryError` inherits `LadingError`; its messages include the git failure detail. diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 9a2e5c0d..90b8fb8f 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -19,12 +19,19 @@ refreshed in place after manifest rewrites, whereas validation here uses ``cargo metadata --locked`` purely as a read-only freshness probe. +The publish pre-flight domain reaches these operations through the +:class:`LockfileInspectionRepository` port (issue #82) rather than holding a +raw command runner. :class:`CargoLockfileInspectionRepository` is the +git- and cargo-backed adapter, bound to a runner (and optional environment +overrides) at the pre-flight composition root. + Typical publish-side usage: ```python -lockfiles = discover_tracked_lockfiles(workspace_root, runner) +repository = CargoLockfileInspectionRepository(runner=runner) +lockfiles = repository.discover_tracked_lockfiles(workspace_root) for lockfile_path in lockfiles: - validate_lockfile_freshness(lockfile_path.parent / "Cargo.toml", runner) + repository.validate_lockfile_freshness(lockfile_path.parent / "Cargo.toml") ``` """ @@ -226,3 +233,70 @@ def _is_lockfile_stale_detail(detail: str) -> bool: "needs to be updated" in normalized or "cannot update the lock file" in normalized ) + + +@dc.dataclass(frozen=True, slots=True) +class CargoLockfileInspectionRepository: + """Git- and cargo-backed adapter for publish-side lockfile inspection. + + Binds a :class:`~lading.runtime.CommandRunner` (and optional environment + overrides) so the publish pre-flight domain can discover tracked lockfiles + and probe their freshness without holding a raw command runner (issue #82). + The adapter applies ``env`` to any invocation that does not supply its own, + matching the behaviour the pre-flight base environment previously wired in + through an inline runner wrapper. + """ + + runner: CommandRunner + env: cabc.Mapping[str, str] | None = None + manifest_exists: _ManifestExists = _manifest_exists + + def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: + """Return tracked Cargo.lock files with adjacent manifests.""" + return discover_tracked_lockfiles( + workspace_root, + self._bound_runner(), + manifest_exists=self.manifest_exists, + ) + + def validate_lockfile_freshness(self, manifest_path: Path) -> LockfileFreshness: + """Return Cargo's locked-mode freshness result for ``manifest_path``.""" + return validate_lockfile_freshness(manifest_path, self._bound_runner()) + + def _bound_runner(self) -> CommandRunner: + """Return ``runner`` with ``env`` applied when a call omits its own.""" + if self.env is None: + return self.runner + base_env = self.env + base_runner = self.runner + + def runner_with_env( + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + env: cabc.Mapping[str, str] | None = None, + echo_stdout: bool = True, + ) -> tuple[int, str, str]: + """Invoke ``base_runner`` with ``base_env`` as the default env.""" + del echo_stdout + effective_env = base_env if env is None else env + return base_runner(command, cwd=cwd, env=effective_env) + + return runner_with_env + + +class LockfileInspectionRepository(typ.Protocol): + """Port for discovering tracked lockfiles and probing their freshness. + + The publish pre-flight domain depends on this protocol rather than on a + command runner, keeping VCS, filesystem, and cargo execution concerns out + of the freshness-classification logic (issue #82). This is the publish-side + counterpart to :class:`lading.commands.bump_lockfiles.LockfileRepository`, + which owns bump-side lockfile projection and regeneration. + """ + + def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: + """Return tracked Cargo.lock files with adjacent manifests.""" + + def validate_lockfile_freshness(self, manifest_path: Path) -> LockfileFreshness: + """Return the freshness result for ``manifest_path``.""" diff --git a/lading/commands/publish_preflight.py b/lading/commands/publish_preflight.py index d70d4619..f7ea5334 100644 --- a/lading/commands/publish_preflight.py +++ b/lading/commands/publish_preflight.py @@ -33,16 +33,14 @@ import typing as typ from pathlib import Path -from lading.commands.lockfile import ( - discover_tracked_lockfiles, - validate_lockfile_freshness, -) +from lading.commands.lockfile import CargoLockfileInspectionRepository from lading.commands.publish_diagnostics import _append_compiletest_diagnostics from lading.commands.publish_errors import PublishPreflightError from lading.commands.publish_execution import _invoke from lading.utils.process import append_detail, command_detail, with_detail if typ.TYPE_CHECKING: + from lading.commands.lockfile import LockfileInspectionRepository from lading.config import CompiletestExtern, LadingConfig from lading.runtime import CommandRunner @@ -129,7 +127,14 @@ def _run_preflight_checks( configuration: LadingConfig, runner: CommandRunner | None = None, ) -> None: - """Execute publish pre-flight checks for ``workspace_root``.""" + """Execute publish pre-flight checks for ``workspace_root``. + + This is the composition root for lockfile inspection: it binds + :class:`CargoLockfileInspectionRepository` to the selected command runner + and the pre-flight base environment (issue #82), so the freshness domain + step runs through the port without holding a raw runner. Tests inject a + port double at the :func:`_validate_lockfile_freshness` seam instead. + """ command_runner = runner or _invoke preflight_config = configuration.preflight base_env = _build_preflight_environment(preflight_config.env_overrides) @@ -145,11 +150,11 @@ def _run_preflight_checks( runner=command_runner, env=base_env, ) - _validate_lockfile_freshness( - workspace_root, + repository = CargoLockfileInspectionRepository( runner=command_runner, env=base_env, ) + _validate_lockfile_freshness(workspace_root, repository=repository) with tempfile.TemporaryDirectory(prefix="lading-preflight-target-") as target: target_path = Path(target) @@ -198,7 +203,7 @@ def _compose_preflight_arguments( def _collect_stale_lockfiles( tracked: cabc.Iterable[Path], - runner: CommandRunner, + repository: LockfileInspectionRepository, ) -> list[Path]: """Classify tracked lockfiles; raise immediately on error, return stale paths. @@ -210,7 +215,7 @@ def _collect_stale_lockfiles( stale: list[Path] = [] for lockfile_path in tracked: manifest_path = lockfile_path.parent / "Cargo.toml" - freshness = validate_lockfile_freshness(manifest_path, runner) + freshness = repository.validate_lockfile_freshness(manifest_path) if freshness.is_fresh: continue if freshness.is_stale: @@ -245,26 +250,17 @@ def _build_stale_lockfile_message(stale_lockfiles: list[Path]) -> str: def _validate_lockfile_freshness( workspace_root: Path, *, - runner: CommandRunner, - env: cabc.Mapping[str, str] | None = None, + repository: LockfileInspectionRepository, ) -> None: - """Fail early when tracked Cargo.lock files are stale.""" - base_env = env - - def runner_with_env( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - echo_stdout: bool = True, - ) -> tuple[int, str, str]: - """Invoke ``runner`` with ``base_env`` applied when no env is supplied.""" - del echo_stdout - effective_env = base_env if env is None else env - return runner(command, cwd=cwd, env=effective_env) - - tracked = discover_tracked_lockfiles(workspace_root, runner_with_env) - stale_lockfiles = _collect_stale_lockfiles(tracked, runner_with_env) + """Fail early when tracked Cargo.lock files are stale. + + Discovery and freshness probing run through ``repository`` (the + :class:`LockfileInspectionRepository` port), so this domain step never + holds a command runner or knows how lockfiles are located and validated + (issue #82). + """ + tracked = repository.discover_tracked_lockfiles(workspace_root) + stale_lockfiles = _collect_stale_lockfiles(tracked, repository) if not stale_lockfiles: LOGGER.info("All %d tracked lockfile(s) are fresh under --locked", len(tracked)) diff --git a/tests/unit/publish/test_preflight_lockfile_validation.py b/tests/unit/publish/test_preflight_lockfile_validation.py index 5c106f2e..e7e3ff53 100644 --- a/tests/unit/publish/test_preflight_lockfile_validation.py +++ b/tests/unit/publish/test_preflight_lockfile_validation.py @@ -1,8 +1,15 @@ -"""Unit tests for _validate_lockfile_freshness pre-flight helper.""" +"""Unit tests for the _validate_lockfile_freshness pre-flight helper. + +These exercise the pre-flight freshness domain step through the +:class:`lading.commands.lockfile.LockfileInspectionRepository` port (issue +#82): tests inject a recording repository double instead of a command runner, +so discovery, classification, and remediation messaging are verified without +touching git or cargo. +""" from __future__ import annotations -import collections.abc as cabc +import dataclasses as dc import typing as typ from pathlib import Path @@ -11,78 +18,74 @@ from lading.commands import lockfile, publish, publish_preflight if typ.TYPE_CHECKING: + import collections.abc as cabc + from syrupy.assertion import SnapshotAssertion +@dc.dataclass +class _RecordingLockfileRepository: + """In-memory ``LockfileInspectionRepository`` double for pre-flight tests.""" + + tracked: tuple[Path, ...] + freshness: cabc.Mapping[Path, lockfile.LockfileFreshness] | None = None + default_freshness: lockfile.LockfileFreshness = dc.field( + default_factory=lambda: lockfile.LockfileFreshness(is_fresh=True) + ) + discovered_roots: list[Path] = dc.field(default_factory=list) + validated_manifests: list[Path] = dc.field(default_factory=list) + + def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: + """Record the discovery call and return the configured lockfiles.""" + self.discovered_roots.append(workspace_root) + return self.tracked + + def validate_lockfile_freshness( + self, manifest_path: Path + ) -> lockfile.LockfileFreshness: + """Record the validation call and return the configured freshness.""" + self.validated_manifests.append(manifest_path) + if self.freshness is not None and manifest_path in self.freshness: + return self.freshness[manifest_path] + return self.default_freshness + + +_STALE_DETAIL = "the lock file Cargo.lock needs to be updated but --locked was passed" + + def test_validate_lockfile_freshness_passes_when_all_lockfiles_are_fresh( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path + tmp_path: Path, ) -> None: - """Fresh tracked lockfiles allow preflight to continue.""" + """Fresh tracked lockfiles allow preflight to continue via the port.""" root_lockfile = tmp_path / "Cargo.lock" nested_lockfile = tmp_path / "tests" / "ui_lints" / "Cargo.lock" - recorded_env: list[cabc.Mapping[str, str] | None] = [] - - monkeypatch.setattr( - publish_preflight, - "discover_tracked_lockfiles", - lambda _root, _runner: (root_lockfile, nested_lockfile), - ) + repository = _RecordingLockfileRepository(tracked=(root_lockfile, nested_lockfile)) - def runner( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - recorded_env.append(env) - return 0, "", "" - - publish_preflight._validate_lockfile_freshness( - tmp_path, - runner=runner, - env={"CARGO_TERM_COLOR": "never"}, - ) + publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository) - assert recorded_env == [{"CARGO_TERM_COLOR": "never"}] * 2 + assert repository.discovered_roots == [tmp_path] + assert repository.validated_manifests == [ + root_lockfile.parent / "Cargo.toml", + nested_lockfile.parent / "Cargo.toml", + ] -def test_validate_lockfile_freshness_reports_stale_lockfiles( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path -) -> None: +def test_validate_lockfile_freshness_reports_stale_lockfiles(tmp_path: Path) -> None: """Stale lockfiles are collected and reported with repair commands.""" root_lockfile = tmp_path / "Cargo.lock" nested_lockfile = tmp_path / "tests" / "ui_lints" / "Cargo.lock" - - monkeypatch.setattr( - publish_preflight, - "discover_tracked_lockfiles", - lambda _root, _runner: (root_lockfile, nested_lockfile), - ) - monkeypatch.setattr( - publish_preflight, - "validate_lockfile_freshness", - lambda _manifest, _runner: lockfile.LockfileFreshness( - is_fresh=False, - is_stale=True, - detail=( - "the lock file Cargo.lock needs to be updated but --locked was passed" - ), + repository = _RecordingLockfileRepository( + tracked=(root_lockfile, nested_lockfile), + default_freshness=lockfile.LockfileFreshness( + is_fresh=False, is_stale=True, detail=_STALE_DETAIL ), ) - def runner( - command: tuple[str, ...], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - return 0, "", "" - with pytest.raises( publish.PublishPreflightError, match="Tracked Cargo\\.lock files are stale", ) as excinfo: - publish_preflight._validate_lockfile_freshness(tmp_path, runner=runner, env={}) + publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository) message = str(excinfo.value) assert str(root_lockfile) in message @@ -98,80 +101,42 @@ def runner( def test_validate_lockfile_freshness_error_snapshot( - monkeypatch: pytest.MonkeyPatch, snapshot: SnapshotAssertion, ) -> None: """Stale lockfile remediation output is locked by snapshot.""" workspace_root = Path("/workspace root") root_lockfile = workspace_root / "Cargo.lock" nested_lockfile = workspace_root / "tests" / "ui_lints" / "Cargo.lock" - - monkeypatch.setattr( - publish_preflight, - "discover_tracked_lockfiles", - lambda _root, _runner: (root_lockfile, nested_lockfile), - ) - monkeypatch.setattr( - publish_preflight, - "validate_lockfile_freshness", - lambda _manifest, _runner: lockfile.LockfileFreshness( - is_fresh=False, - is_stale=True, - detail=( - "the lock file Cargo.lock needs to be updated but --locked was passed" - ), + repository = _RecordingLockfileRepository( + tracked=(root_lockfile, nested_lockfile), + default_freshness=lockfile.LockfileFreshness( + is_fresh=False, is_stale=True, detail=_STALE_DETAIL ), ) - def runner( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - return 0, "", "" - with pytest.raises( publish.PublishPreflightError, match="Tracked Cargo\\.lock files are stale", ) as excinfo: publish_preflight._validate_lockfile_freshness( - workspace_root, runner=runner, env={} + workspace_root, repository=repository ) assert str(excinfo.value) == snapshot() -def test_validate_lockfile_freshness_surfaces_cargo_failures( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path -) -> None: +def test_validate_lockfile_freshness_surfaces_cargo_failures(tmp_path: Path) -> None: """Cargo failures unrelated to stale lockfiles abort with cargo details.""" root_lockfile = tmp_path / "Cargo.lock" - - monkeypatch.setattr( - publish_preflight, - "discover_tracked_lockfiles", - lambda _root, _runner: (root_lockfile,), - ) - monkeypatch.setattr( - publish_preflight, - "validate_lockfile_freshness", - lambda _manifest, _runner: lockfile.LockfileFreshness( - is_fresh=False, - detail="failed to download registry index", + repository = _RecordingLockfileRepository( + tracked=(root_lockfile,), + default_freshness=lockfile.LockfileFreshness( + is_fresh=False, detail="failed to download registry index" ), ) - def runner( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - return 0, "", "" - with pytest.raises( publish.PublishPreflightError, match="failed to download registry index", ): - publish_preflight._validate_lockfile_freshness(tmp_path, runner=runner, env={}) + publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository) diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 9aba79ef..dd25468f 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -432,6 +432,96 @@ def runner( return runner +# --------------------------------------------------------------------------- +# CargoLockfileInspectionRepository adapter (issue #82) +# --------------------------------------------------------------------------- + + +def _recording_runner( + calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]], + *, + exit_code: int = 0, + stdout: str = "", + stderr: str = "", +) -> cabc.Callable[..., tuple[int, str, str]]: + """Return a runner recording each invocation's command, cwd, and env.""" + + def runner( + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + env: cabc.Mapping[str, str] | None = None, + ) -> tuple[int, str, str]: + calls.append((tuple(command), cwd, env)) + return exit_code, stdout, stderr + + return runner + + +def test_adapter_discovers_lockfiles_binding_env(tmp_path: Path) -> None: + """The adapter discovers tracked lockfiles through its bound runner and env.""" + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + env=base_env, + ) + + result = repository.discover_tracked_lockfiles(tmp_path) + + assert result == (tmp_path / "Cargo.lock",) + assert calls == [ + (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env) + ] + + +def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: + """The adapter probes freshness through its bound runner, applying env.""" + manifest_path = tmp_path / "Cargo.toml" + calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls), + env=base_env, + ) + + result = repository.validate_lockfile_freshness(manifest_path) + + assert result.is_fresh + assert len(calls) == 1 + command, cwd, env = calls[0] + assert command[:3] == ("cargo", "metadata", "--locked") + assert cwd == manifest_path.parent + assert env == base_env + + +def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None: + """With no bound env the adapter forwards calls without injecting one.""" + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + ) + + repository.discover_tracked_lockfiles(tmp_path) + + assert calls[0][2] is None + + +def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: + """A custom ``manifest_exists`` predicate filters discovery results.""" + calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + manifest_exists=lambda _manifest: False, + ) + + assert repository.discover_tracked_lockfiles(tmp_path) == () + + @pytest.mark.usefixtures("_metrics_registry") def test_discovery_records_lockfile_count(tmp_path: Path) -> None: """Discovery increments the discovered-lockfiles counter by the count.""" From d7bfce0c9a2a82189d99ad5740f019fdc383fd22 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 23:22:44 +0200 Subject: [PATCH 3/9] Split bump lockfile repository-port tests into their own module Resolve the CodeScene Low Cohesion finding on `tests/unit/test_bump_lockfile_rebuild.py` by moving the injected-port integration tests into a focused module. `_RecordingLockfileRepository`, `test_run_uses_injected_lockfile_repository`, and `test_dry_run_projects_through_lockfile_repository` move verbatim into the new `tests/unit/test_bump_lockfile_repository.py`; the original file keeps the monkeypatch-based rebuild scenarios and `_LockfileSkipScenario`. Imports are trimmed to what each file needs: the new module imports only `cabc`, `pathlib`, `bump`, and the workspace builders, and the original drops the now-unused `collections.abc` import. No production code changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_bump_lockfile_rebuild.py | 67 ------------------ tests/unit/test_bump_lockfile_repository.py | 75 +++++++++++++++++++++ 2 files changed, 75 insertions(+), 67 deletions(-) create mode 100644 tests/unit/test_bump_lockfile_repository.py diff --git a/tests/unit/test_bump_lockfile_rebuild.py b/tests/unit/test_bump_lockfile_rebuild.py index 1f5543b5..b4445a8d 100644 --- a/tests/unit/test_bump_lockfile_rebuild.py +++ b/tests/unit/test_bump_lockfile_rebuild.py @@ -2,7 +2,6 @@ from __future__ import annotations -import collections.abc as cabc import dataclasses as dc import pathlib import typing as typ @@ -187,72 +186,6 @@ def fail_regeneration(*args: object, **kwargs: object) -> typ.NoReturn: assert message == snapshot -class _RecordingLockfileRepository: - """LockfileRepository double recording calls without touching Cargo.""" - - def __init__(self) -> None: - self.resolved: list[tuple[pathlib.Path, tuple[str, ...]]] = [] - self.regenerated: list[tuple[pathlib.Path, tuple[str, ...]]] = [] - - def resolve_lockfile_paths( - self, - workspace_root: pathlib.Path, - lockfile_manifests: cabc.Sequence[str], - ) -> tuple[pathlib.Path, ...]: - self.resolved.append((workspace_root, tuple(lockfile_manifests))) - return (workspace_root / "Cargo.lock",) - - def regenerate_lockfiles( - self, - workspace_root: pathlib.Path, - lockfile_manifests: cabc.Sequence[str], - ) -> tuple[pathlib.Path, ...]: - self.regenerated.append((workspace_root, tuple(lockfile_manifests))) - return (workspace_root / "Cargo.lock",) - - -def test_run_uses_injected_lockfile_repository(tmp_path: pathlib.Path) -> None: - """Bump reaches lockfile operations only through the repository port.""" - workspace = _make_workspace(tmp_path) - repository = _RecordingLockfileRepository() - - message = bump.run( - tmp_path, - "1.2.3", - options=bump.BumpOptions( - configuration=_make_config(), - workspace=workspace, - lockfile_repository=repository, - ), - ) - - assert repository.regenerated == [(tmp_path.resolve(), ())] - assert repository.resolved == [] - assert "Cargo.lock (lockfile)" in message - - -def test_dry_run_projects_through_lockfile_repository( - tmp_path: pathlib.Path, -) -> None: - """Dry runs project lockfile paths without regenerating.""" - workspace = _make_workspace(tmp_path) - repository = _RecordingLockfileRepository() - - bump.run( - tmp_path, - "1.2.3", - options=bump.BumpOptions( - dry_run=True, - configuration=_make_config(), - workspace=workspace, - lockfile_repository=repository, - ), - ) - - assert repository.resolved == [(tmp_path.resolve(), ())] - assert repository.regenerated == [] - - @pytest.mark.parametrize( "scenario", [ diff --git a/tests/unit/test_bump_lockfile_repository.py b/tests/unit/test_bump_lockfile_repository.py new file mode 100644 index 00000000..eb4c00fa --- /dev/null +++ b/tests/unit/test_bump_lockfile_repository.py @@ -0,0 +1,75 @@ +"""Integration tests for the injected LockfileRepository port in :mod:`lading.commands.bump`.""" # noqa: E501 + +from __future__ import annotations + +import collections.abc as cabc +import pathlib + +from lading.commands import bump +from tests.helpers.workspace_builders import _make_config, _make_workspace + + +class _RecordingLockfileRepository: + """LockfileRepository double recording calls without touching Cargo.""" + + def __init__(self) -> None: + self.resolved: list[tuple[pathlib.Path, tuple[str, ...]]] = [] + self.regenerated: list[tuple[pathlib.Path, tuple[str, ...]]] = [] + + def resolve_lockfile_paths( + self, + workspace_root: pathlib.Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[pathlib.Path, ...]: + self.resolved.append((workspace_root, tuple(lockfile_manifests))) + return (workspace_root / "Cargo.lock",) + + def regenerate_lockfiles( + self, + workspace_root: pathlib.Path, + lockfile_manifests: cabc.Sequence[str], + ) -> tuple[pathlib.Path, ...]: + self.regenerated.append((workspace_root, tuple(lockfile_manifests))) + return (workspace_root / "Cargo.lock",) + + +def test_run_uses_injected_lockfile_repository(tmp_path: pathlib.Path) -> None: + """Bump reaches lockfile operations only through the repository port.""" + workspace = _make_workspace(tmp_path) + repository = _RecordingLockfileRepository() + + message = bump.run( + tmp_path, + "1.2.3", + options=bump.BumpOptions( + configuration=_make_config(), + workspace=workspace, + lockfile_repository=repository, + ), + ) + + assert repository.regenerated == [(tmp_path.resolve(), ())] + assert repository.resolved == [] + assert "Cargo.lock (lockfile)" in message + + +def test_dry_run_projects_through_lockfile_repository( + tmp_path: pathlib.Path, +) -> None: + """Dry runs project lockfile paths without regenerating.""" + workspace = _make_workspace(tmp_path) + repository = _RecordingLockfileRepository() + + bump.run( + tmp_path, + "1.2.3", + options=bump.BumpOptions( + dry_run=True, + configuration=_make_config(), + workspace=workspace, + lockfile_repository=repository, + ), + ) + + assert repository.resolved == [(tmp_path.resolve(), ())] + assert repository.regenerated == [] From 269f4e987473c67298ad2323ad3217438a97b6e6 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 11:32:20 +0200 Subject: [PATCH 4/9] Address review feedback on the lockfile repository ports Resolve reviewer findings on the issue #82 lockfile port work: - lockfile.py: `CargoLockfileInspectionRepository._bound_runner` now forwards `echo_stdout` (and any other runner keyword) to the bound runner instead of discarding it; only `env` is defaulted to the bound base environment. Implemented as a transparent `**kwargs` proxy so callers that omit `echo_stdout` leave the underlying runner's default intact and existing runner doubles need no change. - test_lockfile.py: `_recording_runner` now records `echo_stdout`; adds `test_adapter_bound_runner_forwards_echo_stdout`, which drives the env-bound runner with `echo_stdout=False` and asserts it is forwarded unchanged. A `_RecordedCall` type alias keeps the annotations tidy. - bump_lockfiles.py: expand the `LockfileRepository` Protocol and `CargoLockfileRepository` adapter method docstrings to full NumPy-style Parameters/Returns/Raises, documenting the `LockfileRegenerationError` behaviour. (The port methods keep docstring-only bodies; no `...`.) - test_bump_lockfile_repository.py: justify the module-docstring `# noqa: E501` (the one-line summary is fixed; wrapping trips D205/D209). - docs/developers-guide.md: use the file's `issue #82` reference style. - docs/lading-design.md: document the bump- and publish-side lockfile repository ports. The "consolidate the recording runner doubles" suggestion is skipped: the publish `CallTrackingRunner` (a class recording 2-tuples with env discarded, in a non-importable publish conftest) and the lockfile `_recording_runner` are materially different, and merging them would churn three unrelated publish tests for no behavioural gain. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/developers-guide.md | 8 +- docs/lading-design.md | 29 ++++++ lading/commands/bump_lockfiles.py | 106 +++++++++++++++++++- lading/commands/lockfile.py | 12 ++- tests/unit/test_bump_lockfile_repository.py | 2 +- tests/unit/test_lockfile.py | 45 +++++++-- 6 files changed, 180 insertions(+), 22 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 9b346d04..ec07c30e 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -212,7 +212,7 @@ the original cargo error rather than wrapped in the aggregate message. `BumpOptions` carries the dependency-injection points used by `lading.commands.bump.run`. Lockfile operations are reached through the `lockfile_repository` field, a `bump_lockfiles.LockfileRepository` port -introduced by issue 82: the bump domain never holds a raw command runner. When +introduced by issue #82: the bump domain never holds a raw command runner. When the field is `None`, bump uses `bump_lockfiles.CargoLockfileRepository`, the cargo-backed adapter bound to the default subprocess runner; the CLI binds the adapter to its selected runner. Tests inject a repository (or bind the adapter @@ -221,7 +221,7 @@ real Cargo processes. The port's scope is bump-side lockfile projection and regeneration; publish-side discovery and validation go through the sibling `lockfile.LockfileInspectionRepository` port (see the Lockfile helpers section below), so neither the bump nor the publish lockfile domain holds a raw -`CommandRunner` (issue 82). +`CommandRunner` (issue #82). Bump-time crate-set derivation is centralized in the bump context: the `excluded` and `updated_crate_names` sets are computed exactly once in @@ -339,7 +339,7 @@ manifest rewrites, while publish only probes freshness read-only via Cargo says need updating under `--locked`, and unrelated Cargo failures. The publish pre-flight domain reaches both operations through the -`LockfileInspectionRepository` port (issue 82) rather than holding a command +`LockfileInspectionRepository` port (issue #82) rather than holding a command runner: `_validate_lockfile_freshness` and `_collect_stale_lockfiles` in `publish_preflight.py` depend only on the port, so VCS (git), filesystem, and cargo execution concerns stay out of the freshness-classification logic. @@ -351,7 +351,7 @@ adapter to the selected command runner and pre-flight environment. Tests inject a port double at the `_validate_lockfile_freshness` seam, observing discovery and validation without invoking real git or cargo. This is the publish-side counterpart to the bump-side `bump_lockfiles.LockfileRepository`; together they -complete issue 82's separation of lockfile VCS/filesystem concerns from the +complete issue #82's separation of lockfile VCS/filesystem concerns from the command domain. `LockfileDiscoveryError` inherits `LadingError`; its messages include the git diff --git a/docs/lading-design.md b/docs/lading-design.md index 182afeac..419bf8e0 100644 --- a/docs/lading-design.md +++ b/docs/lading-design.md @@ -376,6 +376,19 @@ lading bump [--dry-run] documentation counts, and documentation entries are suffixed with `(documentation)` for clarity. +### Lockfile repository port (bump side) + +The bump domain reaches Cargo lockfile projection and regeneration through +a `LockfileRepository` port defined in +`lading.commands.bump_lockfiles`. `CargoLockfileRepository` is the +cargo-backed adapter; it is constructed with an optional `CommandRunner` +and delegates to the module-level helpers. +`BumpOptions.lockfile_repository` is the injection point; when `None`, +bump substitutes `CargoLockfileRepository` bound to the default +subprocess runner, and the CLI binds the adapter at the composition +root. This keeps the bump domain free of a raw `CommandRunner` +(issue #82). + ## 4. `publish` Subcommand Design The `publish` command orchestrates the publication of crates to the designated @@ -477,6 +490,22 @@ names are listed before returning the user-specified order. compiletest `*.stderr` files when cargo test fails, exposing the debug diff directly in the CLI output. +### Lockfile inspection repository port (publish side) + +The publish pre-flight domain reaches tracked-lockfile discovery and +freshness validation through a `LockfileInspectionRepository` port +defined in `lading.commands.lockfile`. +`CargoLockfileInspectionRepository` is the git- and cargo-backed +adapter; it binds a `CommandRunner` and the optional pre-flight base +environment, applying that environment to invocations that do not +supply their own. `publish_preflight._run_preflight_checks` is the +composition root: it constructs the adapter and passes it to the +domain helpers `_collect_stale_lockfiles` and +`_validate_lockfile_freshness`, which depend only on the port. +Together with the bump-side `LockfileRepository`, the two ports keep +VCS, filesystem, and cargo execution concerns out of the lockfile +domain logic (issue #82). + ### Publish Preflight Sequence The preflight sequence diagram illustrates the pre-flight checks that run diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index 26a82ef2..7f518df9 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -250,7 +250,30 @@ def resolve_lockfile_paths( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], ) -> tuple[Path, ...]: - """Return the lockfile paths a regeneration run would touch.""" + """Return the lockfile paths a regeneration run would touch. + + Parameters + ---------- + workspace_root : + Absolute path to the Cargo workspace root. + lockfile_manifests : + Configured manifest paths relative to *workspace_root*. The + workspace-root ``Cargo.toml`` is always prepended and + de-duplicated. + + Returns + ------- + tuple[Path, ...] + The lockfile paths that a regeneration run would touch, + in manifest execution order. + + Raises + ------ + LockfileRegenerationError + If any configured manifest path is invalid (outside the + workspace or not named ``Cargo.toml``), propagated from + :func:`_resolve_manifest_paths`. + """ return resolve_lockfile_paths(workspace_root, lockfile_manifests) def regenerate_lockfiles( @@ -258,7 +281,33 @@ def regenerate_lockfiles( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], ) -> tuple[Path, ...]: - """Regenerate lockfiles via ``cargo update --workspace``.""" + """Regenerate lockfiles via ``cargo update --workspace``. + + Parameters + ---------- + workspace_root : + Absolute path to the Cargo workspace root. + lockfile_manifests : + Configured manifest paths relative to *workspace_root*. The + workspace-root ``Cargo.toml`` is always prepended and + de-duplicated. + + Returns + ------- + tuple[Path, ...] + Paths to every ``Cargo.lock`` regenerated, in manifest + execution order. + + Raises + ------ + LockfileRegenerationError + If any configured manifest path is invalid (outside the + workspace or not named ``Cargo.toml``), or — after every + manifest has been attempted — if ``cargo update --workspace`` + failed. A lone workspace-root failure re-raises the original + cargo error unchanged; multiple failures raise one aggregated + error listing each failed manifest with a repair command. + """ return regenerate_lockfiles( workspace_root, lockfile_manifests, runner=self.runner ) @@ -277,11 +326,60 @@ def resolve_lockfile_paths( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], ) -> tuple[Path, ...]: - """Return the lockfile paths a regeneration run would touch.""" + """Return the lockfile paths a regeneration run would touch. + + Parameters + ---------- + workspace_root : + Absolute path to the Cargo workspace root. + lockfile_manifests : + Configured manifest paths relative to *workspace_root*. The + workspace-root ``Cargo.toml`` is always prepended and + de-duplicated. + + Returns + ------- + tuple[Path, ...] + The lockfile paths that a regeneration run would touch, + in manifest execution order. + + Raises + ------ + LockfileRegenerationError + If any configured manifest path is invalid (outside the + workspace or not named ``Cargo.toml``), propagated from + :func:`_resolve_manifest_paths`. + """ def regenerate_lockfiles( self, workspace_root: Path, lockfile_manifests: cabc.Sequence[str], ) -> tuple[Path, ...]: - """Regenerate lockfiles and return the rewritten paths.""" + """Regenerate lockfiles and return the rewritten paths. + + Parameters + ---------- + workspace_root : + Absolute path to the Cargo workspace root. + lockfile_manifests : + Configured manifest paths relative to *workspace_root*. The + workspace-root ``Cargo.toml`` is always prepended and + de-duplicated. + + Returns + ------- + tuple[Path, ...] + Paths to every ``Cargo.lock`` regenerated, in manifest + execution order. + + Raises + ------ + LockfileRegenerationError + If any configured manifest path is invalid (outside the + workspace or not named ``Cargo.toml``), or — after every + manifest has been attempted — if ``cargo update --workspace`` + failed. A lone workspace-root failure re-raises the original + cargo error unchanged; multiple failures raise one aggregated + error listing each failed manifest with a repair command. + """ diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 90b8fb8f..e7c35d2c 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -275,12 +275,16 @@ def runner_with_env( *, cwd: Path | None = None, env: cabc.Mapping[str, str] | None = None, - echo_stdout: bool = True, + **runner_kwargs: bool, ) -> tuple[int, str, str]: - """Invoke ``base_runner`` with ``base_env`` as the default env.""" - del echo_stdout + """Invoke ``base_runner`` with ``base_env`` as the default env. + + Any extra keyword (notably ``echo_stdout``) is forwarded to + ``base_runner`` unchanged; only ``env`` is defaulted (to the bound + ``base_env``) when a call omits it. + """ effective_env = base_env if env is None else env - return base_runner(command, cwd=cwd, env=effective_env) + return base_runner(command, cwd=cwd, env=effective_env, **runner_kwargs) return runner_with_env diff --git a/tests/unit/test_bump_lockfile_repository.py b/tests/unit/test_bump_lockfile_repository.py index eb4c00fa..e79fafea 100644 --- a/tests/unit/test_bump_lockfile_repository.py +++ b/tests/unit/test_bump_lockfile_repository.py @@ -1,4 +1,4 @@ -"""Integration tests for the injected LockfileRepository port in :mod:`lading.commands.bump`.""" # noqa: E501 +"""Integration tests for the injected LockfileRepository port in :mod:`lading.commands.bump`.""" # noqa: E501 fixed one-line module docstring; wrapping it trips pydocstyle D205/D209 from __future__ import annotations diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index dd25468f..483e77a7 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -437,22 +437,27 @@ def runner( # --------------------------------------------------------------------------- +# Each recorded call captures (command, cwd, env, echo_stdout). +_RecordedCall = tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None, bool] + + def _recording_runner( - calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]], + calls: list[_RecordedCall], *, exit_code: int = 0, stdout: str = "", stderr: str = "", ) -> cabc.Callable[..., tuple[int, str, str]]: - """Return a runner recording each invocation's command, cwd, and env.""" + """Return a runner recording each invocation's command, cwd, env, echo_stdout.""" def runner( command: cabc.Sequence[str], *, cwd: Path | None = None, env: cabc.Mapping[str, str] | None = None, + echo_stdout: bool = True, ) -> tuple[int, str, str]: - calls.append((tuple(command), cwd, env)) + calls.append((tuple(command), cwd, env, echo_stdout)) return exit_code, stdout, stderr return runner @@ -462,7 +467,7 @@ def test_adapter_discovers_lockfiles_binding_env(tmp_path: Path) -> None: """The adapter discovers tracked lockfiles through its bound runner and env.""" (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") - calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + calls: list[_RecordedCall] = [] base_env = {"CARGO_TERM_COLOR": "never"} repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls, stdout="Cargo.lock\n"), @@ -473,14 +478,14 @@ def test_adapter_discovers_lockfiles_binding_env(tmp_path: Path) -> None: assert result == (tmp_path / "Cargo.lock",) assert calls == [ - (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env) + (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env, True) ] def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: """The adapter probes freshness through its bound runner, applying env.""" manifest_path = tmp_path / "Cargo.toml" - calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + calls: list[_RecordedCall] = [] base_env = {"CARGO_TERM_COLOR": "never"} repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls), @@ -491,17 +496,18 @@ def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: assert result.is_fresh assert len(calls) == 1 - command, cwd, env = calls[0] + command, cwd, env, echo_stdout = calls[0] assert command[:3] == ("cargo", "metadata", "--locked") assert cwd == manifest_path.parent assert env == base_env + assert echo_stdout is True def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None: """With no bound env the adapter forwards calls without injecting one.""" (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") - calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + calls: list[_RecordedCall] = [] repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls, stdout="Cargo.lock\n"), ) @@ -513,7 +519,7 @@ def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: """A custom ``manifest_exists`` predicate filters discovery results.""" - calls: list[tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None]] = [] + calls: list[_RecordedCall] = [] repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls, stdout="Cargo.lock\n"), manifest_exists=lambda _manifest: False, @@ -522,6 +528,27 @@ def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: assert repository.discover_tracked_lockfiles(tmp_path) == () +def test_adapter_bound_runner_forwards_echo_stdout(tmp_path: Path) -> None: + """The env-bound runner forwards ``echo_stdout`` unchanged to the runner.""" + calls: list[_RecordedCall] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls), + env=base_env, + ) + + bound_runner = repository._bound_runner() + bound_runner(("git", "status"), cwd=tmp_path, echo_stdout=False) + + assert len(calls) == 1 + command, cwd, env, echo_stdout = calls[0] + assert command == ("git", "status") + assert cwd == tmp_path + # env is still defaulted from the bound base_env when the call omits it. + assert env == base_env + assert echo_stdout is False + + @pytest.mark.usefixtures("_metrics_registry") def test_discovery_records_lockfile_count(tmp_path: Path) -> None: """Discovery increments the discovered-lockfiles counter by the count.""" From a7ddba47f75d0488a4239c5fc3f50d755b5056ca Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 17:38:25 +0200 Subject: [PATCH 5/9] Polish lockfile port docs and adopt PEP 695 type alias - lockfile.py: add a NumPy-style Attributes section to the CargoLockfileInspectionRepository docstring documenting runner, env, and manifest_exists, matching the BumpOptions documentation style. - test_lockfile.py: convert the _RecordedCall alias to a PEP 695 `type` statement, consistent with the rest of the py313 codebase. Co-Authored-By: Claude Opus 4.8 (1M context) --- lading/commands/lockfile.py | 12 ++++++++++++ tests/unit/test_lockfile.py | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index e7c35d2c..920262f1 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -245,6 +245,18 @@ class CargoLockfileInspectionRepository: The adapter applies ``env`` to any invocation that does not supply its own, matching the behaviour the pre-flight base environment previously wired in through an inline runner wrapper. + + Attributes + ---------- + runner : CommandRunner + Command runner used to execute the git discovery and cargo freshness + probes. + env : Mapping[str, str] | None, default None + Environment overrides applied to any invocation that does not supply + its own; ``None`` leaves each call's environment untouched. + manifest_exists : Callable[[Path], bool], default _manifest_exists + Predicate deciding whether a discovered lockfile has an adjacent + ``Cargo.toml`` manifest; the default checks the filesystem. """ runner: CommandRunner diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 483e77a7..9709c555 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -438,7 +438,9 @@ def runner( # Each recorded call captures (command, cwd, env, echo_stdout). -_RecordedCall = tuple[tuple[str, ...], Path | None, cabc.Mapping[str, str] | None, bool] +type _RecordedCall = tuple[ + tuple[str, ...], Path | None, cabc.Mapping[str, str] | None, bool +] def _recording_runner( From d834defc56bd10b0c43739165b8238f3a57474f9 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 17:41:38 +0200 Subject: [PATCH 6/9] Make injected manifest_exists adapter test non-vacuous test_adapter_honours_injected_manifest_exists previously passed even if the adapter ignored the injected predicate: with no Cargo.toml on disk, the default filesystem probe also returned an empty result. Create a real Cargo.toml/Cargo.lock pair so the default probe would include the lockfile, assert the injected predicate is invoked with the expected manifest path, and assert it filters the lockfile out. The test now fails if the adapter drops manifest_exists. The helper-level probe property test is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_lockfile.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 9709c555..e39470bc 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -520,14 +520,28 @@ def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: - """A custom ``manifest_exists`` predicate filters discovery results.""" + """A custom ``manifest_exists`` predicate overrides the filesystem probe.""" + # Create a real manifest/lockfile pair so the *default* filesystem probe + # would include this lockfile. The injected predicate must be what excludes + # it, so the test fails if the adapter ignores ``manifest_exists``. + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") calls: list[_RecordedCall] = [] + probed: list[Path] = [] + + def manifest_exists(manifest_path: Path) -> bool: + probed.append(manifest_path) + return False + repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls, stdout="Cargo.lock\n"), - manifest_exists=lambda _manifest: False, + manifest_exists=manifest_exists, ) - assert repository.discover_tracked_lockfiles(tmp_path) == () + result = repository.discover_tracked_lockfiles(tmp_path) + + assert result == () + assert probed == [tmp_path / "Cargo.toml"] def test_adapter_bound_runner_forwards_echo_stdout(tmp_path: Path) -> None: From 5a95b0ecc9b99c3d546f702d776beb35aae15ac4 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 18:40:16 +0200 Subject: [PATCH 7/9] Expand lockfile port docstrings and message the adapter-test asserts - lockfile.py: expand the discover_tracked_lockfiles and validate_lockfile_freshness docstrings on both the CargoLockfileInspectionRepository adapter and the LockfileInspectionRepository protocol to full NumPy Parameters/Returns sections (protocol stubs stay docstring-only, no ellipsis). - test_lockfile.py: attach failure messages to the bare asserts in the five CargoLockfileInspectionRepository adapter tests. The suggestion to group the adapter tests under a test class is skipped: the entire test suite is module-level functions (no test classes in any file), so a lone class here would be an inconsistency island; CodeRabbit itself rated it a poor tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) --- lading/commands/lockfile.py | 61 ++++++++++++++++++++++++++++++++++--- tests/unit/test_lockfile.py | 32 +++++++++---------- 2 files changed, 73 insertions(+), 20 deletions(-) diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 920262f1..2b309018 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -264,7 +264,20 @@ class CargoLockfileInspectionRepository: manifest_exists: _ManifestExists = _manifest_exists def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: - """Return tracked Cargo.lock files with adjacent manifests.""" + """Return tracked Cargo.lock files with adjacent manifests. + + Parameters + ---------- + workspace_root + Path to the repository root searched for tracked lockfiles. + + Returns + ------- + tuple[Path, ...] + Git-tracked ``Cargo.lock`` files outside any ``target`` directory + that have an adjacent ``Cargo.toml`` manifest. + + """ return discover_tracked_lockfiles( workspace_root, self._bound_runner(), @@ -272,7 +285,21 @@ def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: ) def validate_lockfile_freshness(self, manifest_path: Path) -> LockfileFreshness: - """Return Cargo's locked-mode freshness result for ``manifest_path``.""" + """Return Cargo's locked-mode freshness result for ``manifest_path``. + + Parameters + ---------- + manifest_path + Path to the Cargo manifest to validate under ``--locked``. + + Returns + ------- + LockfileFreshness + Structured result describing whether the lockfile is fresh, stale + (Cargo says it needs updating under ``--locked``), or failed for + another reason. + + """ return validate_lockfile_freshness(manifest_path, self._bound_runner()) def _bound_runner(self) -> CommandRunner: @@ -312,7 +339,33 @@ class LockfileInspectionRepository(typ.Protocol): """ def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: - """Return tracked Cargo.lock files with adjacent manifests.""" + """Return tracked Cargo.lock files with adjacent manifests. + + Parameters + ---------- + workspace_root + Path to the repository root searched for tracked lockfiles. + + Returns + ------- + tuple[Path, ...] + Git-tracked ``Cargo.lock`` files outside any ``target`` directory + that have an adjacent ``Cargo.toml`` manifest. + + """ def validate_lockfile_freshness(self, manifest_path: Path) -> LockfileFreshness: - """Return the freshness result for ``manifest_path``.""" + """Return the freshness result for ``manifest_path``. + + Parameters + ---------- + manifest_path + Path to the Cargo manifest to validate. + + Returns + ------- + LockfileFreshness + The freshness result, distinguishing fresh, stale, and failed + states. + + """ diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index e39470bc..91f2de15 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -478,10 +478,10 @@ def test_adapter_discovers_lockfiles_binding_env(tmp_path: Path) -> None: result = repository.discover_tracked_lockfiles(tmp_path) - assert result == (tmp_path / "Cargo.lock",) + assert result == (tmp_path / "Cargo.lock",), "discovers the tracked lockfile" assert calls == [ (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env, True) - ] + ], "git ls-files should receive the bound env" def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: @@ -496,13 +496,13 @@ def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: result = repository.validate_lockfile_freshness(manifest_path) - assert result.is_fresh - assert len(calls) == 1 + assert result.is_fresh, "probe should report the lockfile fresh" + assert len(calls) == 1, "one cargo call expected" command, cwd, env, echo_stdout = calls[0] - assert command[:3] == ("cargo", "metadata", "--locked") - assert cwd == manifest_path.parent - assert env == base_env - assert echo_stdout is True + assert command[:3] == ("cargo", "metadata", "--locked"), "cargo metadata probe" + assert cwd == manifest_path.parent, "cargo runs in the manifest directory" + assert env == base_env, "cargo call should receive the bound env" + assert echo_stdout is True, "echo_stdout defaults to True" def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None: @@ -516,7 +516,7 @@ def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None repository.discover_tracked_lockfiles(tmp_path) - assert calls[0][2] is None + assert calls[0][2] is None, "no env should be injected without a bound env" def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: @@ -540,8 +540,8 @@ def manifest_exists(manifest_path: Path) -> bool: result = repository.discover_tracked_lockfiles(tmp_path) - assert result == () - assert probed == [tmp_path / "Cargo.toml"] + assert result == (), "injected predicate should exclude the lockfile" + assert probed == [tmp_path / "Cargo.toml"], "predicate probed the manifest" def test_adapter_bound_runner_forwards_echo_stdout(tmp_path: Path) -> None: @@ -556,13 +556,13 @@ def test_adapter_bound_runner_forwards_echo_stdout(tmp_path: Path) -> None: bound_runner = repository._bound_runner() bound_runner(("git", "status"), cwd=tmp_path, echo_stdout=False) - assert len(calls) == 1 + assert len(calls) == 1, "one forwarded call expected" command, cwd, env, echo_stdout = calls[0] - assert command == ("git", "status") - assert cwd == tmp_path + assert command == ("git", "status"), "command forwarded unchanged" + assert cwd == tmp_path, "cwd forwarded unchanged" # env is still defaulted from the bound base_env when the call omits it. - assert env == base_env - assert echo_stdout is False + assert env == base_env, "env defaulted from the bound base_env" + assert echo_stdout is False, "echo_stdout forwarded unchanged" @pytest.mark.usefixtures("_metrics_registry") From 7c3e81bd3d1a4c7666af012dd4ac6599f84219a0 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 20:30:45 +0200 Subject: [PATCH 8/9] Group CargoLockfileInspectionRepository adapter tests in a class Move the five test_adapter_* functions into a new TestCargoLockfileInspectionRepositoryAdapter class as methods, keeping their names, docstrings, assertions, and helper usage (_recording_runner, _RecordedCall) unchanged. The _recording_runner factory and _RecordedCall alias stay at module scope since they are shared across the file. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_lockfile.py | 176 +++++++++++++++++++----------------- 1 file changed, 91 insertions(+), 85 deletions(-) diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 91f2de15..32c933e0 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -465,104 +465,110 @@ def runner( return runner -def test_adapter_discovers_lockfiles_binding_env(tmp_path: Path) -> None: - """The adapter discovers tracked lockfiles through its bound runner and env.""" - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") - calls: list[_RecordedCall] = [] - base_env = {"CARGO_TERM_COLOR": "never"} - repository = lockfile.CargoLockfileInspectionRepository( - runner=_recording_runner(calls, stdout="Cargo.lock\n"), - env=base_env, - ) - - result = repository.discover_tracked_lockfiles(tmp_path) - - assert result == (tmp_path / "Cargo.lock",), "discovers the tracked lockfile" - assert calls == [ - (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env, True) - ], "git ls-files should receive the bound env" - - -def test_adapter_validates_freshness_binding_env(tmp_path: Path) -> None: - """The adapter probes freshness through its bound runner, applying env.""" - manifest_path = tmp_path / "Cargo.toml" - calls: list[_RecordedCall] = [] - base_env = {"CARGO_TERM_COLOR": "never"} - repository = lockfile.CargoLockfileInspectionRepository( - runner=_recording_runner(calls), - env=base_env, - ) - - result = repository.validate_lockfile_freshness(manifest_path) - - assert result.is_fresh, "probe should report the lockfile fresh" - assert len(calls) == 1, "one cargo call expected" - command, cwd, env, echo_stdout = calls[0] - assert command[:3] == ("cargo", "metadata", "--locked"), "cargo metadata probe" - assert cwd == manifest_path.parent, "cargo runs in the manifest directory" - assert env == base_env, "cargo call should receive the bound env" - assert echo_stdout is True, "echo_stdout defaults to True" - +class TestCargoLockfileInspectionRepositoryAdapter: + """Tests for the CargoLockfileInspectionRepository adapter (issue #82).""" + + def test_adapter_discovers_lockfiles_binding_env(self, tmp_path: Path) -> None: + """The adapter discovers tracked lockfiles through its bound runner and env.""" + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + calls: list[_RecordedCall] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + env=base_env, + ) -def test_adapter_without_env_leaves_runner_env_untouched(tmp_path: Path) -> None: - """With no bound env the adapter forwards calls without injecting one.""" - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") - calls: list[_RecordedCall] = [] - repository = lockfile.CargoLockfileInspectionRepository( - runner=_recording_runner(calls, stdout="Cargo.lock\n"), - ) + result = repository.discover_tracked_lockfiles(tmp_path) - repository.discover_tracked_lockfiles(tmp_path) + assert result == (tmp_path / "Cargo.lock",), "discovers the tracked lockfile" + assert calls == [ + ( + ("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), + tmp_path, + base_env, + True, + ) + ], "git ls-files should receive the bound env" + + def test_adapter_validates_freshness_binding_env(self, tmp_path: Path) -> None: + """The adapter probes freshness through its bound runner, applying env.""" + manifest_path = tmp_path / "Cargo.toml" + calls: list[_RecordedCall] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls), + env=base_env, + ) - assert calls[0][2] is None, "no env should be injected without a bound env" + result = repository.validate_lockfile_freshness(manifest_path) + + assert result.is_fresh, "probe should report the lockfile fresh" + assert len(calls) == 1, "one cargo call expected" + command, cwd, env, echo_stdout = calls[0] + assert command[:3] == ("cargo", "metadata", "--locked"), "cargo metadata probe" + assert cwd == manifest_path.parent, "cargo runs in the manifest directory" + assert env == base_env, "cargo call should receive the bound env" + assert echo_stdout is True, "echo_stdout defaults to True" + + def test_adapter_without_env_leaves_runner_env_untouched( + self, tmp_path: Path + ) -> None: + """With no bound env the adapter forwards calls without injecting one.""" + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + calls: list[_RecordedCall] = [] + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + ) + repository.discover_tracked_lockfiles(tmp_path) -def test_adapter_honours_injected_manifest_exists(tmp_path: Path) -> None: - """A custom ``manifest_exists`` predicate overrides the filesystem probe.""" - # Create a real manifest/lockfile pair so the *default* filesystem probe - # would include this lockfile. The injected predicate must be what excludes - # it, so the test fails if the adapter ignores ``manifest_exists``. - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") - calls: list[_RecordedCall] = [] - probed: list[Path] = [] + assert calls[0][2] is None, "no env should be injected without a bound env" - def manifest_exists(manifest_path: Path) -> bool: - probed.append(manifest_path) - return False + def test_adapter_honours_injected_manifest_exists(self, tmp_path: Path) -> None: + """A custom ``manifest_exists`` predicate overrides the filesystem probe.""" + # Create a real manifest/lockfile pair so the *default* filesystem probe + # would include this lockfile. The injected predicate must be what excludes + # it, so the test fails if the adapter ignores ``manifest_exists``. + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + calls: list[_RecordedCall] = [] + probed: list[Path] = [] - repository = lockfile.CargoLockfileInspectionRepository( - runner=_recording_runner(calls, stdout="Cargo.lock\n"), - manifest_exists=manifest_exists, - ) + def manifest_exists(manifest_path: Path) -> bool: + probed.append(manifest_path) + return False - result = repository.discover_tracked_lockfiles(tmp_path) + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls, stdout="Cargo.lock\n"), + manifest_exists=manifest_exists, + ) - assert result == (), "injected predicate should exclude the lockfile" - assert probed == [tmp_path / "Cargo.toml"], "predicate probed the manifest" + result = repository.discover_tracked_lockfiles(tmp_path) + assert result == (), "injected predicate should exclude the lockfile" + assert probed == [tmp_path / "Cargo.toml"], "predicate probed the manifest" -def test_adapter_bound_runner_forwards_echo_stdout(tmp_path: Path) -> None: - """The env-bound runner forwards ``echo_stdout`` unchanged to the runner.""" - calls: list[_RecordedCall] = [] - base_env = {"CARGO_TERM_COLOR": "never"} - repository = lockfile.CargoLockfileInspectionRepository( - runner=_recording_runner(calls), - env=base_env, - ) + def test_adapter_bound_runner_forwards_echo_stdout(self, tmp_path: Path) -> None: + """The env-bound runner forwards ``echo_stdout`` unchanged to the runner.""" + calls: list[_RecordedCall] = [] + base_env = {"CARGO_TERM_COLOR": "never"} + repository = lockfile.CargoLockfileInspectionRepository( + runner=_recording_runner(calls), + env=base_env, + ) - bound_runner = repository._bound_runner() - bound_runner(("git", "status"), cwd=tmp_path, echo_stdout=False) + bound_runner = repository._bound_runner() + bound_runner(("git", "status"), cwd=tmp_path, echo_stdout=False) - assert len(calls) == 1, "one forwarded call expected" - command, cwd, env, echo_stdout = calls[0] - assert command == ("git", "status"), "command forwarded unchanged" - assert cwd == tmp_path, "cwd forwarded unchanged" - # env is still defaulted from the bound base_env when the call omits it. - assert env == base_env, "env defaulted from the bound base_env" - assert echo_stdout is False, "echo_stdout forwarded unchanged" + assert len(calls) == 1, "one forwarded call expected" + command, cwd, env, echo_stdout = calls[0] + assert command == ("git", "status"), "command forwarded unchanged" + assert cwd == tmp_path, "cwd forwarded unchanged" + # env is still defaulted from the bound base_env when the call omits it. + assert env == base_env, "env defaulted from the bound base_env" + assert echo_stdout is False, "echo_stdout forwarded unchanged" @pytest.mark.usefixtures("_metrics_registry") From 2ac681f2e1aa483e593e899f4bb959c7158e1411 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 21:04:40 +0200 Subject: [PATCH 9/9] Extract shared Cargo workspace setup into a fixture Three CargoLockfileInspectionRepository adapter tests wrote the same root Cargo.toml/Cargo.lock pair inline. Introduce a module-local _cargo_workspace pytest fixture (mirroring the existing _metrics_registry pattern) and apply it via @pytest.mark.usefixtures on the three tests that need the pair, dropping the duplicated write_text calls. Each test keeps its tmp_path parameter and assertions unchanged; the two adapter tests that do not need a workspace on disk are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_lockfile.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 32c933e0..5f5e9f38 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -465,13 +465,19 @@ def runner( return runner +@pytest.fixture +def _cargo_workspace(tmp_path: Path) -> None: + """Write a minimal root Cargo workspace manifest and empty lockfile.""" + (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") + (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + + class TestCargoLockfileInspectionRepositoryAdapter: """Tests for the CargoLockfileInspectionRepository adapter (issue #82).""" + @pytest.mark.usefixtures("_cargo_workspace") def test_adapter_discovers_lockfiles_binding_env(self, tmp_path: Path) -> None: """The adapter discovers tracked lockfiles through its bound runner and env.""" - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") calls: list[_RecordedCall] = [] base_env = {"CARGO_TERM_COLOR": "never"} repository = lockfile.CargoLockfileInspectionRepository( @@ -511,12 +517,11 @@ def test_adapter_validates_freshness_binding_env(self, tmp_path: Path) -> None: assert env == base_env, "cargo call should receive the bound env" assert echo_stdout is True, "echo_stdout defaults to True" + @pytest.mark.usefixtures("_cargo_workspace") def test_adapter_without_env_leaves_runner_env_untouched( self, tmp_path: Path ) -> None: """With no bound env the adapter forwards calls without injecting one.""" - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") calls: list[_RecordedCall] = [] repository = lockfile.CargoLockfileInspectionRepository( runner=_recording_runner(calls, stdout="Cargo.lock\n"), @@ -526,13 +531,13 @@ def test_adapter_without_env_leaves_runner_env_untouched( assert calls[0][2] is None, "no env should be injected without a bound env" + @pytest.mark.usefixtures("_cargo_workspace") def test_adapter_honours_injected_manifest_exists(self, tmp_path: Path) -> None: """A custom ``manifest_exists`` predicate overrides the filesystem probe.""" - # Create a real manifest/lockfile pair so the *default* filesystem probe - # would include this lockfile. The injected predicate must be what excludes - # it, so the test fails if the adapter ignores ``manifest_exists``. - (tmp_path / "Cargo.toml").write_text("[workspace]\n", encoding="utf-8") - (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") + # The _cargo_workspace fixture writes a real manifest/lockfile pair, so the + # default filesystem probe would include this lockfile. The injected + # predicate must be what excludes it, so the test fails if the adapter + # ignores ``manifest_exists``. calls: list[_RecordedCall] = [] probed: list[Path] = []