#829: Fix metric AST contract test#1244
Conversation
📝 WalkthroughWalkthroughThe test ChangesTest config update
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/config/test_config.py (2)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFiltering by
metrics_excludeweakens contract enforcement without guardrails.Any metric code added to
patterns_config['metrics_exclude'](aibolit/config.py) now silently escapes this AST-contract check, with no assertion that the exclusion list itself stays bounded or that excluded metrics are tracked/justified. A future contributor could bypass this test entirely for a non-compliant metric by adding its code tometrics_exclude, without any reviewer being alerted by the test itself.Consider asserting that the exclusion list doesn't grow unexpectedly (e.g., pin the exact expected excluded set, or require each excluded code to reference a tracking issue) so regressions in coverage are caught.
🤖 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 `@test/config/test_config.py` around lines 21 - 27, The AST contract test now skips any metric listed in metrics_exclude, which weakens coverage without any safeguard. Tighten test_config.py by asserting the exact expected excluded set (or otherwise validating each excluded code is explicitly justified/tracked) before building ast_compatible_metrics, so metric exclusions in patterns_config cannot silently bypass the check.
10-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate stale docstring.
The docstring still says "Once all metrics meet requirements, remove the decorator," but the
@pytest.mark.xfaildecorator has already been removed in this change. The TODO reference and instructions no longer match the current test behavior (filtering bymetrics_excluderather than requiring all metrics to comply).📝 Suggested docstring update
''' - TODO `#813`:30min/DEV Ensure All Metrics Accept `ast: AST` Parameter with Type Hints - Verify that all metrics in the config accept an AST parameter with proper type hints. - Probable solutions: - 1. Every metric factory in patterns_config["metrics"] produces a metric with: - - A parameter named "ast" in its call signature - - The "ast" parameter properly annotated as `aibolit.ast_framework.ast.AST` or a subclass - 2. Remove any metrics that cannot comply with this interface. - Once all metrics meet requirements, remove the decorator. + Verify that metrics not listed in `metrics_exclude` accept an AST + parameter with proper type hints (named "ast", annotated as + `aibolit.ast_framework.ast.AST`). '''🤖 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 `@test/config/test_config.py` around lines 10 - 19, Update the stale docstring in the test helper around the metrics validation logic so it matches the current behavior. Remove the outdated xfail/decorator language and rewrite the TODO to describe the actual check performed by the test, which now filters metrics via metrics_exclude and verifies the remaining metric factories’ ast parameter signature and type hints. Keep the references to the metrics_config/patterns_config validation flow and the test function context clear so the docstring aligns with the current implementation.
🤖 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.
Nitpick comments:
In `@test/config/test_config.py`:
- Around line 21-27: The AST contract test now skips any metric listed in
metrics_exclude, which weakens coverage without any safeguard. Tighten
test_config.py by asserting the exact expected excluded set (or otherwise
validating each excluded code is explicitly justified/tracked) before building
ast_compatible_metrics, so metric exclusions in patterns_config cannot silently
bypass the check.
- Around line 10-19: Update the stale docstring in the test helper around the
metrics validation logic so it matches the current behavior. Remove the outdated
xfail/decorator language and rewrite the TODO to describe the actual check
performed by the test, which now filters metrics via metrics_exclude and
verifies the remaining metric factories’ ast parameter signature and type hints.
Keep the references to the metrics_config/patterns_config validation flow and
the test function context clear so the docstring aligns with the current
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 639aa193-2dab-4e5c-89cb-071f7640f6a7
📒 Files selected for processing (1)
test/config/test_config.py
3 participants
Summary by CodeRabbit