Fix changed-file code quality checks - #2178
Conversation
Use the checked-out synthetic merge parent and compare base/head mypy diagnostics so stale payload bases and inherited errors cannot fail unrelated pull requests. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR replaces payload-based mypy diffing with merge-parent comparison. It adds Git-aware changed-file tracking, isolated base and head mypy runs, rename-aware diagnostic filtering, and CI and pre-commit validation. ChangesMypy quality ratchet
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This PR corrects changed-file code-quality comparisons and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CodeQualityWorkflow
participant pre_commit_diff
participant GitRepository
participant mypy
CodeQualityWorkflow->>pre_commit_diff: invoke changed-file mypy check
pre_commit_diff->>GitRepository: resolve HEAD^1 and HEAD
pre_commit_diff->>GitRepository: create base and head checkouts
pre_commit_diff->>mypy: run checks for changed Python files
mypy-->>pre_commit_diff: return diagnostics
pre_commit_diff-->>CodeQualityWorkflow: fail when new diagnostics exist
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/tools/ci/test_mypy_diff.py (3)
45-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the rejection branch.
_parse_changed_python_filesraisesValueErrorfor unsupportedgit name-statuslines atnoxfile.pyline 292. No test covers that branch. A malformed status line would then fail the whole CI lane with an unclear error.A single negative case documents the contract.
🧪 Suggested additional test
import pytest def test_parse_changed_python_files_rejects_unsupported_status(): with pytest.raises(ValueError, match="Unsupported git name-status line"): _parse_changed_python_files("D\tdeleted.py")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/tools/ci/test_mypy_diff.py` around lines 45 - 62, Add a negative test for _parse_changed_python_files that passes an unsupported status line such as “D\tdeleted.py” and asserts pytest.raises(ValueError, match="Unsupported git name-status line").
137-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind the assertion to the literal
HEAD^1expression.The test computes
current_baseby recording the target-branch SHA before the merge. The workflow instead passesHEAD^1at.github/workflows/code_quality.ymlline 41. The test therefore validates the premise but not the expression the workflow uses.Resolving
merge_head^1in the test ties the two together. A future change to first-parent selection then fails this test.🧪 Suggested assertion
_git(tmp_path, "merge", "--no-ff", "topic", "-m", "synthetic pull request merge") merge_head = _git(tmp_path, "rev-parse", "HEAD") + + # The workflow passes HEAD^1; confirm it resolves to the recorded target snapshot. + assert _git(tmp_path, "rev-parse", f"{merge_head}^1") == current_base🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/tools/ci/test_mypy_diff.py` around lines 137 - 158, Update the test around current_base and merge_head to derive the exact-base comparison from merge_head^1 (HEAD^1) after the synthetic merge, rather than capturing the pre-merge SHA separately. Keep the stale_base comparison unchanged and use the resolved first-parent SHA when constructing exact_output so the test remains bound to the workflow expression.
166-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert configuration structure instead of exact substrings and pinned versions.
Both tests match raw file text. Two problems follow.
First, the assertions break on formatting changes that do not change behavior. A reflowed
argslist or a re-quotedrunline fails the test.Second,
test_mypy_hook_pins_stubs_and_disables_automatic_installationasserts exact versions ofnox,types-docutils, andtypes-PyYAML. Every routine dependency bump then fails this test. The useful invariant is that each dependency carries a==pin, not that it carries one specific version. As written the test blocks upgrades rather than protecting against a regression.Parse the YAML and assert on the resolved structure.
🧪 Suggested structural assertions
import yaml def test_mypy_hook_pins_stubs_and_disables_automatic_installation(): config = yaml.safe_load( (REPOSITORY_ROOT / ".pre-commit-config.yaml").read_text(encoding="utf-8") ) hook = next( hook for repo in config["repos"] for hook in repo["hooks"] if hook["id"] == "mypy" ) assert "--no-install-types" in hook["args"] dependencies = hook["additional_dependencies"] assert {name.split("==")[0] for name in dependencies} == { "nox", "types-docutils", "types-PyYAML", } # Every stub dependency must be pinned so hook runs stay reproducible. assert all("==" in name for name in dependencies)As per path instructions, "checked-in tests should be lean and document expected behavior, protect against regressions, or flag backward-incompatible changes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/tools/ci/test_mypy_diff.py` around lines 166 - 179, Update the configuration tests to parse YAML with yaml.safe_load and assert resolved structure rather than raw text. In test_code_quality_uses_checked_out_pull_request_merge_parent, inspect the parsed workflow command while preserving the HEAD^1/HEAD behavior and rejecting github.event.pull_request.base.sha. In test_mypy_hook_pins_stubs_and_disables_automatic_installation, locate the hook by id "mypy", verify --no-install-types, require the expected dependency names, and assert every dependency uses a == pin without hard-coding versions.Source: Path instructions
noxfile.py (1)
366-367: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the base-versus-head contract of
_run_changed_file_mypy.Add a short docstring stating that the helper runs mypy on both snapshots and reports only new diagnostics. Keep
--follow-imports=skip; the previouspre_commit_diffimplementation already used this option, so this change preserves the existing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@noxfile.py` around lines 366 - 367, Add a concise docstring to the _run_changed_file_mypy helper documenting that it runs mypy against both base and head snapshots and reports only newly introduced diagnostics. Preserve the existing --follow-imports=skip option and related behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/tools/ci/test_mypy_diff.py`:
- Around line 24-31: Update the pytest configuration in pyproject.toml to
include the repository root alongside tests/ in pythonpath, so direct collection
of test_mypy_diff.py can resolve imports from noxfile.
---
Nitpick comments:
In `@noxfile.py`:
- Around line 366-367: Add a concise docstring to the _run_changed_file_mypy
helper documenting that it runs mypy against both base and head snapshots and
reports only newly introduced diagnostics. Preserve the existing
--follow-imports=skip option and related behavior unchanged.
In `@tests/unit/tools/ci/test_mypy_diff.py`:
- Around line 45-62: Add a negative test for _parse_changed_python_files that
passes an unsupported status line such as “D\tdeleted.py” and asserts
pytest.raises(ValueError, match="Unsupported git name-status line").
- Around line 137-158: Update the test around current_base and merge_head to
derive the exact-base comparison from merge_head^1 (HEAD^1) after the synthetic
merge, rather than capturing the pre-merge SHA separately. Keep the stale_base
comparison unchanged and use the resolved first-parent SHA when constructing
exact_output so the test remains bound to the workflow expression.
- Around line 166-179: Update the configuration tests to parse YAML with
yaml.safe_load and assert resolved structure rather than raw text. In
test_code_quality_uses_checked_out_pull_request_merge_parent, inspect the parsed
workflow command while preserving the HEAD^1/HEAD behavior and rejecting
github.event.pull_request.base.sha. In
test_mypy_hook_pins_stubs_and_disables_automatic_installation, locate the hook
by id "mypy", verify --no-install-types, require the expected dependency names,
and assert every dependency uses a == pin without hard-coding versions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1275020-d00b-4b3e-bd35-fc5bc9809b8c
📒 Files selected for processing (4)
.github/workflows/code_quality.yml.pre-commit-config.yamlnoxfile.pytests/unit/tools/ci/test_mypy_diff.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/puzzletron_v2 #2178 +/- ##
=========================================================
+ Coverage 53.15% 62.38% +9.22%
=========================================================
Files 704 705 +1
Lines 91506 91574 +68
=========================================================
+ Hits 48640 57126 +8486
+ Misses 42866 34448 -8418
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Keep direct pytest collection aligned with the documented invocation and preserve the mypy hook arguments while allowing routine pinned dependency updates. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
|
1 participant
What does this PR do?
Type of change: Bug fix
The changed-file code-quality job currently reads the pull request base SHA from the event payload. That value can lag the synthetic merge checked out for a run, causing target-only files and inherited mypy diagnostics to be attributed to an unrelated pull request.
This change binds the comparison to the checked-out merge's first parent, pins the mypy hook dependencies used by the diff session, and compares normalized base and head diagnostics so only newly introduced errors fail the gate. Renames and copies are handled explicitly, and repository-controlled paths are separated from mypy options.
Testing
The code-quality workflow gains focused coverage for merge-parent selection, changed-file status handling, rename-aware diagnostic comparison, multiplicity, dependency pinning, and option-safe paths.
Summary by CodeRabbit
Bug Fixes
Chores