From 1f9cac41d76e60cf0bb8eb8c6550b3fb67d8ab54 Mon Sep 17 00:00:00 2001 From: mac Date: Wed, 24 Jun 2026 22:05:28 +0800 Subject: [PATCH 1/5] Log warnings on silent patch step skips --- cli/localci/core/patch_pipeline.py | 7 +++ cli/localci/core/patch_steps.py | 94 +++++++++++++++++------------- cli/tests/test_patch_pipeline.py | 23 +++++++- cli/tests/test_patcher_patches.py | 89 ++++++++++++++++++++++------ 4 files changed, 152 insertions(+), 61 deletions(-) diff --git a/cli/localci/core/patch_pipeline.py b/cli/localci/core/patch_pipeline.py index 7249f29..a4b35ab 100644 --- a/cli/localci/core/patch_pipeline.py +++ b/cli/localci/core/patch_pipeline.py @@ -2,12 +2,15 @@ from __future__ import annotations +import logging from abc import ABC, abstractmethod from dataclasses import dataclass from typing import TYPE_CHECKING from localci.core.config import PATCH_STEP_NAMES, LocalCIConfig +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from localci.core.workflow import MatrixEntry @@ -24,6 +27,10 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: """Apply this patch in place to ``ctx.lines``.""" + def _skip(self, reason: str) -> None: + """Log a warning when this step exits without modifying the workflow.""" + logger.warning("Patch step %r skipped: %s", self.name, reason) + @dataclass class PatchContext: diff --git a/cli/localci/core/patch_steps.py b/cli/localci/core/patch_steps.py index 0346524..3e268a3 100644 --- a/cli/localci/core/patch_steps.py +++ b/cli/localci/core/patch_steps.py @@ -41,7 +41,7 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.job_id or not ctx.container_mount_options: - return + return self._skip("job_id and container_mount_options are required") job_header = re.compile(r"^\s{2}" + re.escape(ctx.job_id) + r"\s*:\s*$") for i, line in enumerate(ctx.lines): if not job_header.match(line): @@ -69,8 +69,10 @@ def apply(self, ctx: PatchContext) -> None: j + 1, f'{options_indent}options: "{ctx.container_mount_options}"\n', ) - break - break + return + self._skip(f"job {ctx.job_id!r} has no container block") + return + self._skip(f"job {ctx.job_id!r} not found in workflow") class B2SourceCacheStep(PatchStep): @@ -100,7 +102,8 @@ def apply(self, ctx: PatchContext) -> None: f"{ind} fi\n" f"{ind}fi\n" ) - break + return + self._skip("no cp -rL boost-source boost-root line found") class RestoreCapyTimestampsStep(PatchStep): @@ -118,24 +121,27 @@ def apply(self, ctx: PatchContext) -> None: already_patched = any( "capy-file-stats" in ctx.lines[j] for j in range(max(0, i - 15), i) ) - if not already_patched: - list_indent = step_match.group(1) - prop_indent = list_indent + " " - body_indent = prop_indent + " " - new_step = [ - f"{list_indent}- name: Restore capy source file timestamps\n", - f"{prop_indent}run: |\n", - f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats" ]; then\n', - f"{body_indent} while IFS=' ' read -r saved_mtime fhash relpath; do\n", - f'{body_indent} [ -f "capy-root/$relpath" ] || continue\n', - f"{body_indent} curr=$(sha256sum \"capy-root/$relpath\" 2>/dev/null | cut -d' ' -f1)\n", - f'{body_indent} [ "$curr" = "$fhash" ] && touch -d "@$saved_mtime" "capy-root/$relpath" 2>/dev/null || true\n', - f'{body_indent} done < "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n', - f"{body_indent}fi\n", - ] - for j, new_line in enumerate(new_step): - ctx.lines.insert(i + j, new_line) - break + if already_patched: + self._skip("restore step already present") + return + list_indent = step_match.group(1) + prop_indent = list_indent + " " + body_indent = prop_indent + " " + new_step = [ + f"{list_indent}- name: Restore capy source file timestamps\n", + f"{prop_indent}run: |\n", + f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats" ]; then\n', + f"{body_indent} while IFS=' ' read -r saved_mtime fhash relpath; do\n", + f'{body_indent} [ -f "capy-root/$relpath" ] || continue\n', + f"{body_indent} curr=$(sha256sum \"capy-root/$relpath\" 2>/dev/null | cut -d' ' -f1)\n", + f'{body_indent} [ "$curr" = "$fhash" ] && touch -d "@$saved_mtime" "capy-root/$relpath" 2>/dev/null || true\n', + f'{body_indent} done < "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n', + f"{body_indent}fi\n", + ] + for j, new_line in enumerate(new_step): + ctx.lines.insert(i + j, new_line) + return + self._skip("Patch Boost step not found") class CapyCopyPreservationStep(PatchStep): @@ -161,7 +167,8 @@ def apply(self, ctx: PatchContext) -> None: f'{ind} done > "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n' f"{ind}fi\n" ) - break + return + self._skip('no cp -r "$workspace_root" capy copy line found') class B2BootstrapSkipStep(PatchStep): @@ -183,21 +190,24 @@ def apply(self, ctx: PatchContext) -> None: "Skip b2 bootstrap" in ctx.lines[j] for j in range(max(0, step_start - 15), step_start) ) - if not already_patched: - list_indent, prop_indent, body_indent = _step_insert_indents( - ctx.lines, step_start - ) - new_step = [ - f"{list_indent}- name: Skip b2 bootstrap (b2 binary cached)\n", - f"{prop_indent}run: |\n", - f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/b2" ]; then\n', - f"{body_indent} printf '#!/bin/sh\\necho \"b2 binary cached, skipping bootstrap.\"\\n' > boost-root/bootstrap.sh\n", - f"{body_indent} chmod +x boost-root/bootstrap.sh\n", - f"{body_indent}fi\n", - ] - for j, new_line in enumerate(new_step): - ctx.lines.insert(step_start + j, new_line) - break + if already_patched: + self._skip("skip b2 bootstrap step already present") + return + list_indent, prop_indent, body_indent = _step_insert_indents( + ctx.lines, step_start + ) + new_step = [ + f"{list_indent}- name: Skip b2 bootstrap (b2 binary cached)\n", + f"{prop_indent}run: |\n", + f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/b2" ]; then\n', + f"{body_indent} printf '#!/bin/sh\\necho \"b2 binary cached, skipping bootstrap.\"\\n' > boost-root/bootstrap.sh\n", + f"{body_indent} chmod +x boost-root/bootstrap.sh\n", + f"{body_indent}fi\n", + ] + for j, new_line in enumerate(new_step): + ctx.lines.insert(step_start + j, new_line) + return + self._skip("no b2-workflow uses step found") class ImageSubstitutionStep(PatchStep): @@ -209,7 +219,7 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.image_tag: - return + return self._skip("image_tag not provided") name_escaped = re.escape(ctx.entry.name) name_pattern = re.compile(r'name:\s*["\']?' + name_escaped + r'["\']?\s*$') name_idx = None @@ -255,7 +265,8 @@ def apply(self, ctx: PatchContext) -> None: mo = container_pattern.match(ctx.lines[i]) if mo: ctx.lines[i] = f'{mo.group(1)}container: "{ctx.image_tag}"\n' - break + return + self._skip("container field not found in matrix entry block") class CodecovSkipStep(PatchStep): @@ -280,7 +291,8 @@ def apply(self, ctx: PatchContext) -> None: f"{indent}{act_check}{rest}; " f'else echo "Skipping Codecov upload (running under act)."; fi\n' ) - break + return + self._skip("no Codecov upload step found") PATCH_STEP_REGISTRY: dict[str, type[PatchStep]] = { diff --git a/cli/tests/test_patch_pipeline.py b/cli/tests/test_patch_pipeline.py index 50de1d3..e4aeeac 100644 --- a/cli/tests/test_patch_pipeline.py +++ b/cli/tests/test_patch_pipeline.py @@ -2,13 +2,15 @@ from __future__ import annotations +import logging from pathlib import Path import pytest from localci.cli.run.patcher import _write_patched_workflow from localci.core.config import LocalCIConfig, PatchesConfig -from localci.core.patch_pipeline import PatchPipeline +from localci.core.patch_pipeline import PatchContext, PatchPipeline +from localci.core.patch_steps import ContainerMountsStep from localci.core.workflow import ( BuildSystem, BuildVariant, @@ -192,3 +194,22 @@ def test_patches_config_rejects_enabled_step_missing_from_order() -> None: "image_substitution", ], ) + + +def test_patch_step_skip_emits_warning(sample_entry: MatrixEntry, caplog) -> None: + """Enabled patch step with incomplete context logs a skip warning.""" + ctx = PatchContext( + lines=["jobs:\n", " build:\n", " runs-on: ubuntu-latest\n"], + entry=sample_entry, + config=LocalCIConfig(), + job_id=None, + container_mount_options=None, + ) + with caplog.at_level(logging.WARNING, logger="localci.core.patch_pipeline"): + ContainerMountsStep().apply(ctx) + + assert any( + "container_mounts" in r.message + and "job_id and container_mount_options are required" in r.message + for r in caplog.records + ) diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index eda0ec6..0441afa 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -5,6 +5,7 @@ from __future__ import annotations +import logging from pathlib import Path import pytest @@ -23,6 +24,16 @@ ) FIXTURES_DIR = Path(__file__).parent / "fixtures" / "patcher" +PATCH_LOGGER = "localci.core.patch_pipeline" + + +def _assert_skip_warning(caplog, step_name: str, reason_fragment: str) -> None: + assert any( + step_name in r.message and reason_fragment in r.message for r in caplog.records + ), ( + f"expected skip warning for {step_name!r} containing {reason_fragment!r}, " + f"got: {[r.message for r in caplog.records]}" + ) def _make_entry(name: str = "GCC 15: C++20") -> MatrixEntry: @@ -104,12 +115,14 @@ def test_negative_raises_when_matrix_entry_name_missing(self, patcher_paths): image_tag="localci/missing:latest", ) - def test_negative_skips_when_image_tag_not_provided(self, patcher_paths): + def test_negative_skips_when_image_tag_not_provided(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_image.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert 'container: "ubuntu:25.04"' in content assert "localci/" not in content + _assert_skip_warning(caplog, "image_substitution", "image_tag not provided") # --------------------------------------------------------------------------- @@ -142,22 +155,34 @@ def test_positive_injects_mount_options( if has_existing_options: assert "--privileged" in content - def test_negative_skips_without_job_id(self, patcher_paths): + def test_negative_skips_without_job_id(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_mount.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths( - workflow, - container_mount_options="-v /host/boost:/cache", - ) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + container_mount_options="-v /host/boost:/cache", + ) content = _assert_valid_yaml(patched) assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, + "container_mounts", + "job_id and container_mount_options are required", + ) - def test_negative_skips_without_mount_options(self, patcher_paths): + def test_negative_skips_without_mount_options(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_mount.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths(workflow, job_id="build") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow, job_id="build") content = _assert_valid_yaml(patched) assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, + "container_mounts", + "job_id and container_mount_options are required", + ) # --------------------------------------------------------------------------- @@ -177,15 +202,19 @@ def test_positive_replaces_cp_with_cache_logic(self, patcher_paths): assert "cp -a boost-root/." in content def test_negative_leaves_workflow_unchanged_without_boost_copy_line( - self, patcher_paths + self, patcher_paths, caplog ): workflow = FIXTURES_DIR / "container_image.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert "LOCALCI_B2_SOURCE_DIR" not in content assert "cp -rL boost-source boost-root" not in content assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "b2_source_cache", "no cp -rL boost-source boost-root line found" + ) # --------------------------------------------------------------------------- @@ -205,11 +234,17 @@ def test_positive_injects_restore_step_before_patch_boost(self, patcher_paths): "Patch Boost" ) - def test_negative_skips_duplicate_on_already_patched_workflow(self, patcher_paths): + def test_negative_skips_duplicate_on_already_patched_workflow( + self, patcher_paths, caplog + ): workflow = FIXTURES_DIR / "already_patched.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert content.count("Restore capy source file timestamps") == 1 + _assert_skip_warning( + caplog, "restore_capy_timestamps", "restore step already present" + ) # --------------------------------------------------------------------------- @@ -228,11 +263,19 @@ def test_positive_replaces_cp_r_with_cp_rp_and_stats(self, patcher_paths): assert "sha256sum" in content assert 'cp -r "$workspace_root"' not in content - def test_negative_no_cp_rp_injected_when_capy_copy_line_absent(self, patcher_paths): + def test_negative_no_cp_rp_injected_when_capy_copy_line_absent( + self, patcher_paths, caplog + ): workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert 'cp -rp "$workspace_root"' not in content + _assert_skip_warning( + caplog, + "capy_copy_preservation", + 'no cp -r "$workspace_root" capy copy line found', + ) # --------------------------------------------------------------------------- @@ -250,11 +293,15 @@ def test_positive_injects_skip_step_before_b2_workflow(self, patcher_paths): assert "bootstrap.sh" in content assert content.index("Skip b2 bootstrap") < content.index("b2-workflow") - def test_negative_skips_when_b2_workflow_step_absent(self, patcher_paths): + def test_negative_skips_when_b2_workflow_step_absent(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert "Skip b2 bootstrap (b2 binary cached)" not in content + _assert_skip_warning( + caplog, "b2_bootstrap_skip", "no b2-workflow uses step found" + ) # --------------------------------------------------------------------------- @@ -272,13 +319,17 @@ def test_positive_wraps_upload_with_act_guard(self, patcher_paths): assert "Skipping Codecov upload (running under act)" in content assert "https://codecov.io/bash" in content - def test_negative_leaves_workflow_without_codecov_unchanged(self, patcher_paths): + def test_negative_leaves_workflow_without_codecov_unchanged( + self, patcher_paths, caplog + ): workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert "codecov.io" not in content assert "ACT" not in content assert "LOCALCI_B2_SOURCE_DIR" in content + _assert_skip_warning(caplog, "codecov_skip", "no Codecov upload step found") # --------------------------------------------------------------------------- From adc79c14ec963e8d9eb8e6615d00911d94a6f665 Mon Sep 17 00:00:00 2001 From: mac Date: Wed, 24 Jun 2026 22:55:33 +0800 Subject: [PATCH 2/5] addressed ai reviews --- cli/localci/core/patch_pipeline.py | 2 +- cli/localci/core/patch_steps.py | 20 ++++++- .../container_mount_no_container_block.yml | 8 +++ cli/tests/test_patch_pipeline.py | 3 +- cli/tests/test_patcher_patches.py | 60 ++++++++++++++++++- 5 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 cli/tests/fixtures/patcher/container_mount_no_container_block.yml diff --git a/cli/localci/core/patch_pipeline.py b/cli/localci/core/patch_pipeline.py index a4b35ab..0d2fce0 100644 --- a/cli/localci/core/patch_pipeline.py +++ b/cli/localci/core/patch_pipeline.py @@ -29,7 +29,7 @@ def apply(self, ctx: PatchContext) -> None: def _skip(self, reason: str) -> None: """Log a warning when this step exits without modifying the workflow.""" - logger.warning("Patch step %r skipped: %s", self.name, reason) + logger.warning("Patch step %s skipped: %s", self.name, reason) @dataclass diff --git a/cli/localci/core/patch_steps.py b/cli/localci/core/patch_steps.py index 3e268a3..c8dd2e3 100644 --- a/cli/localci/core/patch_steps.py +++ b/cli/localci/core/patch_steps.py @@ -41,7 +41,8 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.job_id or not ctx.container_mount_options: - return self._skip("job_id and container_mount_options are required") + self._skip("job_id and container_mount_options are required") + return job_header = re.compile(r"^\s{2}" + re.escape(ctx.job_id) + r"\s*:\s*$") for i, line in enumerate(ctx.lines): if not job_header.match(line): @@ -73,6 +74,7 @@ def apply(self, ctx: PatchContext) -> None: self._skip(f"job {ctx.job_id!r} has no container block") return self._skip(f"job {ctx.job_id!r} not found in workflow") + return class B2SourceCacheStep(PatchStep): @@ -104,6 +106,7 @@ def apply(self, ctx: PatchContext) -> None: ) return self._skip("no cp -rL boost-source boost-root line found") + return class RestoreCapyTimestampsStep(PatchStep): @@ -142,6 +145,7 @@ def apply(self, ctx: PatchContext) -> None: ctx.lines.insert(i + j, new_line) return self._skip("Patch Boost step not found") + return class CapyCopyPreservationStep(PatchStep): @@ -169,6 +173,7 @@ def apply(self, ctx: PatchContext) -> None: ) return self._skip('no cp -r "$workspace_root" capy copy line found') + return class B2BootstrapSkipStep(PatchStep): @@ -208,10 +213,16 @@ def apply(self, ctx: PatchContext) -> None: ctx.lines.insert(step_start + j, new_line) return self._skip("no b2-workflow uses step found") + return class ImageSubstitutionStep(PatchStep): - """Replace matrix container image with the locally-built image tag.""" + """Replace matrix container image with the locally-built image tag. + + Raises ValueError if the matrix entry name is not found (unexpected workflow + structure). Skips with a warning if the container field is absent from the + entry block (valid but unsupported layout). + """ @property def name(self) -> str: @@ -219,7 +230,8 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.image_tag: - return self._skip("image_tag not provided") + self._skip("image_tag not provided") + return name_escaped = re.escape(ctx.entry.name) name_pattern = re.compile(r'name:\s*["\']?' + name_escaped + r'["\']?\s*$') name_idx = None @@ -267,6 +279,7 @@ def apply(self, ctx: PatchContext) -> None: ctx.lines[i] = f'{mo.group(1)}container: "{ctx.image_tag}"\n' return self._skip("container field not found in matrix entry block") + return class CodecovSkipStep(PatchStep): @@ -293,6 +306,7 @@ def apply(self, ctx: PatchContext) -> None: ) return self._skip("no Codecov upload step found") + return PATCH_STEP_REGISTRY: dict[str, type[PatchStep]] = { diff --git a/cli/tests/fixtures/patcher/container_mount_no_container_block.yml b/cli/tests/fixtures/patcher/container_mount_no_container_block.yml new file mode 100644 index 0000000..4d00cbb --- /dev/null +++ b/cli/tests/fixtures/patcher/container_mount_no_container_block.yml @@ -0,0 +1,8 @@ +name: Container Mount No Container Block +on: + push: +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo build diff --git a/cli/tests/test_patch_pipeline.py b/cli/tests/test_patch_pipeline.py index e4aeeac..f45d0ed 100644 --- a/cli/tests/test_patch_pipeline.py +++ b/cli/tests/test_patch_pipeline.py @@ -209,7 +209,8 @@ def test_patch_step_skip_emits_warning(sample_entry: MatrixEntry, caplog) -> Non ContainerMountsStep().apply(ctx) assert any( - "container_mounts" in r.message + r.levelno == logging.WARNING + and "container_mounts" in r.message and "job_id and container_mount_options are required" in r.message for r in caplog.records ) diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index 0441afa..2a1a313 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -12,6 +12,7 @@ import yaml from localci.cli.run import _write_patched_workflow +from localci.core.config import LocalCIConfig, PatchesConfig from localci.core.workflow import ( BuildSystem, BuildVariant, @@ -29,10 +30,27 @@ def _assert_skip_warning(caplog, step_name: str, reason_fragment: str) -> None: assert any( - step_name in r.message and reason_fragment in r.message for r in caplog.records + r.levelno == logging.WARNING + and step_name in r.message + and reason_fragment in r.message + for r in caplog.records ), ( - f"expected skip warning for {step_name!r} containing {reason_fragment!r}, " - f"got: {[r.message for r in caplog.records]}" + f"expected WARNING for {step_name!r} containing {reason_fragment!r}, " + f"got: {[(r.levelname, r.message) for r in caplog.records]}" + ) + + +def _container_mounts_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=True, + b2_source_cache=False, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) ) @@ -184,6 +202,40 @@ def test_negative_skips_without_mount_options(self, patcher_paths, caplog): "job_id and container_mount_options are required", ) + def test_negative_skips_when_job_has_no_container_block( + self, patcher_paths, caplog + ): + workflow = FIXTURES_DIR / "container_mount_no_container_block.yml" + original = workflow.read_text(encoding="utf-8") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + job_id="build", + container_mount_options="-v /host/boost:/cache", + config=_container_mounts_only_config(), + ) + content = _assert_valid_yaml(patched) + assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "container_mounts", "job 'build' has no container block" + ) + + def test_negative_skips_when_job_not_found(self, patcher_paths, caplog): + workflow = FIXTURES_DIR / "container_mount.yml" + original = workflow.read_text(encoding="utf-8") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + job_id="missing_job", + container_mount_options="-v /host/boost:/cache", + config=_container_mounts_only_config(), + ) + content = _assert_valid_yaml(patched) + assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "container_mounts", "job 'missing_job' not found in workflow" + ) + # --------------------------------------------------------------------------- # boost-source cache (cp -rL replacement) @@ -266,6 +318,8 @@ def test_positive_replaces_cp_r_with_cp_rp_and_stats(self, patcher_paths): def test_negative_no_cp_rp_injected_when_capy_copy_line_absent( self, patcher_paths, caplog ): + # Full default pipeline on boost_cache.yml skips several steps; we assert + # only the capy_copy_preservation warning for this fixture. workflow = FIXTURES_DIR / "boost_cache.yml" with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): patched = patcher_paths(workflow) From 6cbc009e9705094693f03e99fa5a3c4252dbce60 Mon Sep 17 00:00:00 2001 From: mac Date: Wed, 24 Jun 2026 23:03:54 +0800 Subject: [PATCH 3/5] - Added two targeted tests with single-step configs to avoid unrelated warnings - Extended PatchStep.apply docstring in patch_pipeline.py to state that early exits that do not modify ctx.lines must call _skip(reason). --- cli/localci/core/patch_pipeline.py | 6 +- .../patcher/container_image_no_container.yml | 13 ++++ cli/tests/test_patcher_patches.py | 59 +++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 cli/tests/fixtures/patcher/container_image_no_container.yml diff --git a/cli/localci/core/patch_pipeline.py b/cli/localci/core/patch_pipeline.py index 0d2fce0..886b296 100644 --- a/cli/localci/core/patch_pipeline.py +++ b/cli/localci/core/patch_pipeline.py @@ -25,7 +25,11 @@ def name(self) -> str: @abstractmethod def apply(self, ctx: PatchContext) -> None: - """Apply this patch in place to ``ctx.lines``.""" + """Apply this patch in place to ``ctx.lines``. + + Early exits that do not modify ``ctx.lines`` must call + :meth:`_skip` with an actionable reason. + """ def _skip(self, reason: str) -> None: """Log a warning when this step exits without modifying the workflow.""" diff --git a/cli/tests/fixtures/patcher/container_image_no_container.yml b/cli/tests/fixtures/patcher/container_image_no_container.yml new file mode 100644 index 0000000..2877fcd --- /dev/null +++ b/cli/tests/fixtures/patcher/container_image_no_container.yml @@ -0,0 +1,13 @@ +name: Container Image No Container Field +on: + push: +jobs: + build: + strategy: + matrix: + include: + - name: "GCC 15: C++20" + runs-on: ubuntu-latest + runs-on: ${{ matrix.runs-on }} + steps: + - run: echo build diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index 2a1a313..115ab30 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -54,6 +54,34 @@ def _container_mounts_only_config() -> LocalCIConfig: ) +def _restore_capy_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=False, + restore_capy_timestamps=True, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) + ) + + +def _image_substitution_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=False, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=True, + codecov_skip=False, + ) + ) + + def _make_entry(name: str = "GCC 15: C++20") -> MatrixEntry: return MatrixEntry( index=0, @@ -142,6 +170,25 @@ def test_negative_skips_when_image_tag_not_provided(self, patcher_paths, caplog) assert "localci/" not in content _assert_skip_warning(caplog, "image_substitution", "image_tag not provided") + def test_negative_skips_when_matrix_entry_has_no_container_field( + self, patcher_paths, caplog + ): + workflow = FIXTURES_DIR / "container_image_no_container.yml" + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + _make_entry("GCC 15: C++20"), + image_tag="localci/gcc-15:custom", + config=_image_substitution_only_config(), + ) + content = _assert_valid_yaml(patched) + assert "localci/gcc-15:custom" not in content + _assert_skip_warning( + caplog, + "image_substitution", + "container field not found in matrix entry block", + ) + # --------------------------------------------------------------------------- # container mount options (cache injection) @@ -298,6 +345,18 @@ def test_negative_skips_duplicate_on_already_patched_workflow( caplog, "restore_capy_timestamps", "restore step already present" ) + def test_negative_skips_when_patch_boost_step_absent( + self, patcher_paths, caplog + ): + workflow = FIXTURES_DIR / "container_image.yml" + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow, config=_restore_capy_only_config()) + content = _assert_valid_yaml(patched) + assert "Restore capy source file timestamps" not in content + _assert_skip_warning( + caplog, "restore_capy_timestamps", "Patch Boost step not found" + ) + # --------------------------------------------------------------------------- # capy copy + stats (cp -r -> cp -rp) From b1d443889f9674ce8c75c4d0186ae3ba894e3833 Mon Sep 17 00:00:00 2001 From: mac Date: Wed, 24 Jun 2026 23:06:35 +0800 Subject: [PATCH 4/5] fixed a lint error --- cli/tests/test_patcher_patches.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index 115ab30..9c1050a 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -345,9 +345,7 @@ def test_negative_skips_duplicate_on_already_patched_workflow( caplog, "restore_capy_timestamps", "restore step already present" ) - def test_negative_skips_when_patch_boost_step_absent( - self, patcher_paths, caplog - ): + def test_negative_skips_when_patch_boost_step_absent(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_image.yml" with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): patched = patcher_paths(workflow, config=_restore_capy_only_config()) From 9c341c622a20e583196ab2e1137f7fb666f62410 Mon Sep 17 00:00:00 2001 From: mac Date: Thu, 25 Jun 2026 23:58:40 +0800 Subject: [PATCH 5/5] added _b2_source_cache_only_config for addressing the review --- cli/tests/test_patcher_patches.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index 9c1050a..8b126ec 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -82,6 +82,20 @@ def _image_substitution_only_config() -> LocalCIConfig: ) +def _b2_source_cache_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=True, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) + ) + + def _make_entry(name: str = "GCC 15: C++20") -> MatrixEntry: return MatrixEntry( index=0, @@ -306,7 +320,7 @@ def test_negative_leaves_workflow_unchanged_without_boost_copy_line( workflow = FIXTURES_DIR / "container_image.yml" original = workflow.read_text(encoding="utf-8") with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): - patched = patcher_paths(workflow) + patched = patcher_paths(workflow, config=_b2_source_cache_only_config()) content = _assert_valid_yaml(patched) assert "LOCALCI_B2_SOURCE_DIR" not in content assert "cp -rL boost-source boost-root" not in content