Log warnings on silent patch step skips#67
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesPatch skip logging
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai, full review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== patch_pipeline.py ==="
cat -n cli/localci/core/patch_pipeline.py
echo ""
echo "=== patch_steps.py ==="
cat -n cli/localci/core/patch_steps.py
echo ""
echo "=== test_patch_pipeline.py ==="
cat -n cli/tests/test_patch_pipeline.py
echo ""
echo "=== test_patcher_patches.py ==="
cat -n cli/tests/test_patcher_patches.pyLength of output: 44930
Overall AssessmentThe PR cleanly achieves its goal: every silent early-return in the patch pipeline now emits a
|
| # | Condition | Message | Tested? |
|---|---|---|---|
| 1 | not ctx.job_id or not ctx.container_mount_options |
job_id and container_mount_options are required |
✅ |
| 2 | Job header found, but no container: block below it |
job {job_id!r} has no container block |
❌ |
| 3 | Outer loop exhausted — job header never found | job {job_id!r} not found in workflow |
❌ |
Paths 2 and 3 are exercised by production workflows but have no unit tests. A minimal fixture (a job with job_id set but no container: section) would cover both.
Missing trailing return after _skip at the end of the outer loop in ContainerMountsStep
Line 75:
self._skip(f"job {ctx.job_id!r} not found in workflow")
# no return here — but line 74 above has oneThis is fine because it is the last statement, but it reads inconsistently alongside line 74 which has an explicit return. If you standardise on Style 2, add return here too.
ImageSubstitutionStep: asymmetric failure modes
When ctx.entry.name is not in the workflow, the step raises ValueError (line 231). When the container: field is missing from the located block, it calls _skip (line 269). This asymmetry is not new to this PR, but since you are now making skip/raise behaviour visible through warnings, a short docstring explaining the distinction would help future maintainers:
class ImageSubstitutionStep(PatchStep):
"""Replace matrix container image with the locally-built image tag.
Raises ValueError if the matrix entry name is not found (unexpected workflow
structure). Skips silently if the container field is absent from the entry
block (valid but unsupported layout).
"""cli/tests/test_patch_pipeline.py
test_patch_step_skip_emits_warning — consider checking r.levelno
The assertion only checks message content, not log level:
assert any(
"container_mounts" in r.message
and "job_id and container_mount_options are required" in r.message
for r in caplog.records
)If _skip were accidentally changed to logger.info instead of logger.warning, this test would still pass. Adding a level check makes the test a complete specification:
assert any(
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
)The same applies to _assert_skip_warning in test_patcher_patches.py.
cli/tests/test_patcher_patches.py
_assert_skip_warning does not assert log level
Same concern as above — the helper should also assert r.levelno == logging.WARNING:
def _assert_skip_warning(caplog, step_name: str, reason_fragment: str) -> None:
assert any(
r.levelno == logging.WARNING
and step_name in r.message
and reason_fragment in 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]}"
)test_negative_leaves_workflow_unchanged_without_boost_copy_line checks multiple skip warnings in one run
This test uses boost_cache.yml and calls patcher_paths(workflow) with the full default pipeline. The caplog block will capture warnings from all steps that skip on this fixture (e.g., capy_copy_preservation, b2_bootstrap_skip, codecov_skip), not just b2_source_cache. The test correctly asserts one specific warning, so it works — but the error message on failure lists all captured records, which could be confusing. A comment explaining why multiple skip warnings are expected would help.
Summary of Actionable Items
| Priority | Item |
|---|---|
| Medium | Standardise _skip call-site style to one pattern across all steps |
| Medium | Add tests for ContainerMountsStep paths 2 & 3 (job found but no container; job not found) |
| Low | Assert r.levelno == logging.WARNING in _assert_skip_warning and test_patch_step_skip_emits_warning |
| Low | Switch %r to %s for step name in the _skip log message, or document the %r choice |
| Low | Add a docstring to ImageSubstitutionStep explaining the raise-vs-skip distinction |
The PR is in good shape — no functional bugs, the coverage improvements are real, and the _skip abstraction is a clean design. The items above are mostly polish and test completeness.
…d warnings - Extended PatchStep.apply docstring in patch_pipeline.py to state that early exits that do not modify ctx.lines must call _skip(reason).
|
@coderabbitai review |
✅ Action performedReview finished.
|
bradjin8
left a comment
bradjin8
left a comment
There was a problem hiding this comment.
Summary
Clean implementation of #65 via PatchStep._skip(reason) (Option B). All seven patch steps instrumented; tests assert WARNING level and skip reasons. Author addressed CodeRabbit follow-ups in adc79c1–b1d4438 (%s log format, WARNING level checks, ContainerMounts paths 2/3, single-step test configs, apply() docstring).
Test plan
- CI green
-
pytest tests/test_patch_pipeline.py tests/test_patcher_patches.py— 44 passed locally
| assert "cp -a boost-root/." in content | ||
|
|
There was a problem hiding this comment.
Nit: test_negative_leaves_workflow_unchanged_without_boost_copy_line still uses the full default pipeline; other new negatives use single-step configs
3 participants
Summary
PatchStep._skip(reason)on the patch pipeline base class to emitlogging.warningwith the step name and skip reason.container_mounts,b2_source_cache,restore_capy_timestamps,capy_copy_preservation,b2_bootstrap_skip,image_substitution,codecov_skip) on every no-op exit path.test_patcher_patches.pyand addtest_patch_step_skip_emits_warningintest_patch_pipeline.py.Patch steps that returned without changing workflow YAML gave no diagnostic when config was incomplete or target content was missing.
Test plan
pytest tests/test_patch_pipeline.py tests/test_patcher_patches.pypytest -m "not integration"ruff check/ruff format --check/mypy localci/core/patch_pipeline.py localci/core/patch_steps.pyRelated issue
Summary by CodeRabbit