#952: Report NPath complexity by method#1246
Conversation
📝 WalkthroughWalkthroughAdds a fallback path in the NPath metric to compute complexity without Maven when ChangesMaven-free NPath metric and reporting
Re-enabling skipped and xfailed tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: PoemA rabbit hopped where 🚥 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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/config/test_config.py (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring/TODO after scope change.
The TODO still says "Once all metrics meet requirements, remove the decorator." but the decorator is already removed and the test now permanently narrows its scope via
metrics_excluderather than covering all metrics. The comment should be updated to reflect that excluded metrics are intentionally not verified going forward, otherwise the docstring misleads future readers about the test's actual coverage.🤖 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 9 - 19, Update the docstring/TODO in test_each_metric_in_config_accepts_ast to match the current test behavior: it no longer removes a decorator after all metrics comply, and it now intentionally uses metrics_exclude to narrow coverage. Clarify that excluded metrics are intentionally not verified by this test going forward, and remove the outdated wording about removing the decorator. Refer to test_each_metric_in_config_accepts_ast and metrics_exclude when revising the text.
🤖 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 `@aibolit/metrics/npath/main.py`:
- Around line 67-73: _`_value_without_maven()` should preserve the same input
handling as `value()` and not reject directory inputs._ Update the input
validation in `_value_without_maven` to accept both `os.path.isdir(self.input)`
and `os.path.isfile(self.input)`, matching the behavior in `NPathMetric.value`.
Then make sure the AST construction path can handle either case consistently
before calling `MvnFreeNPathMetric.value()`.
- Around line 110-122: The report generation in report() and the aggregate in
value() are skipping methods declared inside nested classes because
AST.subtrees(ASTNodeType.CLASS_DECLARATION) and class_declaration.methods only
cover direct class methods. Update the traversal in NPathCalculator so nested
class methods are either included intentionally or explicitly excluded with the
correct behavior, and add a regression test covering an inner class to verify
the expected result.
In `@test/config/test_config.py`:
- Around line 21-27: The test setup is incorrectly filtering metrics using
metrics_exclude, which can hide valid AST-compatible metrics like
NumberMethods/M8. Update the logic in the test_config setup to select metrics
based on AST compatibility instead of reusing metrics_exclude, and reference the
existing ast_compatible_metrics loop in test_config.py to either iterate over
all metrics or maintain a separate list of AST-incompatible metrics.
In `@test/metrics/npath/test_all_types.py`:
- Around line 16-24: The no-mvn branch in testIncorrectFormat uses a blanket
Exception assertion, which is too loose. Tighten the pytest.raises call to match
the specific message raised by NPathMetric.value/_value_without_maven, similar
to how the mvn branch matches PMDException. Use the known “File ... does not
exist” message pattern so the test verifies the intended failure path without
accepting unrelated exceptions.
---
Nitpick comments:
In `@test/config/test_config.py`:
- Around line 9-19: Update the docstring/TODO in
test_each_metric_in_config_accepts_ast to match the current test behavior: it no
longer removes a decorator after all metrics comply, and it now intentionally
uses metrics_exclude to narrow coverage. Clarify that excluded metrics are
intentionally not verified by this test going forward, and remove the outdated
wording about removing the decorator. Refer to
test_each_metric_in_config_accepts_ast and metrics_exclude when revising the
text.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b42d55ee-8d5f-4e4c-b59d-9c05ea106c9c
⛔ Files ignored due to path filters (1)
test/stats/results_test.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
aibolit/metrics/npath/main.pytest/config/test_config.pytest/metrics/npath/test_all_types.pytest/recommend/test_recommend_pipeline.pytest/stats/test_stats.py
| def _value_without_maven(self): | ||
| if not os.path.isfile(self.input): | ||
| raise Exception(' '.join(['File', self.input, 'does not exist'])) | ||
| ast = AST.build_from_javalang(build_ast(self.input)) | ||
| complexity = MvnFreeNPathMetric(ast).value() | ||
| return {'data': [{'file': self.input, 'complexity': complexity}], 'errors': []} | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
_value_without_maven() drops directory-input support that the mvn path provides.
The mvn path (value(), lines 43-48) supports both os.path.isdir(self.input) and os.path.isfile(self.input). _value_without_maven() only checks os.path.isfile, so a directory input that previously worked (with mvn installed) will now raise 'File ... does not exist' whenever mvn is unavailable — same input, different outcome depending on the environment.
🐛 Sketch of a fix supporting directory input
def _value_without_maven(self):
- if not os.path.isfile(self.input):
- raise Exception(' '.join(['File', self.input, 'does not exist']))
- ast = AST.build_from_javalang(build_ast(self.input))
- complexity = MvnFreeNPathMetric(ast).value()
- return {'data': [{'file': self.input, 'complexity': complexity}], 'errors': []}
+ if os.path.isfile(self.input):
+ files = [self.input]
+ elif os.path.isdir(self.input):
+ files = [
+ os.path.join(dirpath, name)
+ for dirpath, _, names in os.walk(self.input)
+ for name in names if name.endswith('.java')
+ ]
+ else:
+ raise Exception(' '.join(['File', self.input, 'does not exist']))
+ data = []
+ for f in files:
+ ast = AST.build_from_javalang(build_ast(f))
+ complexity = MvnFreeNPathMetric(ast).value()
+ data.append({'file': f, 'complexity': complexity})
+ return {'data': data, 'errors': []}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _value_without_maven(self): | |
| if not os.path.isfile(self.input): | |
| raise Exception(' '.join(['File', self.input, 'does not exist'])) | |
| ast = AST.build_from_javalang(build_ast(self.input)) | |
| complexity = MvnFreeNPathMetric(ast).value() | |
| return {'data': [{'file': self.input, 'complexity': complexity}], 'errors': []} | |
| def _value_without_maven(self): | |
| if os.path.isfile(self.input): | |
| files = [self.input] | |
| elif os.path.isdir(self.input): | |
| files = [ | |
| os.path.join(dirpath, name) | |
| for dirpath, _, names in os.walk(self.input) | |
| for name in names if name.endswith('.java') | |
| ] | |
| else: | |
| raise Exception(' '.join(['File', self.input, 'does not exist'])) | |
| data = [] | |
| for f in files: | |
| ast = AST.build_from_javalang(build_ast(f)) | |
| complexity = MvnFreeNPathMetric(ast).value() | |
| data.append({'file': f, 'complexity': complexity}) | |
| return {'data': data, 'errors': []} |
🤖 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 `@aibolit/metrics/npath/main.py` around lines 67 - 73,
_`_value_without_maven()` should preserve the same input handling as `value()`
and not reject directory inputs._ Update the input validation in
`_value_without_maven` to accept both `os.path.isdir(self.input)` and
`os.path.isfile(self.input)`, matching the behavior in `NPathMetric.value`. Then
make sure the AST construction path can handle either case consistently before
calling `MvnFreeNPathMetric.value()`.
| return sum(method['complexity'] for method in self.report()) | ||
|
|
||
| def report(self) -> list[NPathMethodReport]: | ||
| return [ | ||
| { | ||
| 'class_name': class_declaration.name, | ||
| 'method_name': method.name, | ||
| 'complexity': self._method_npath(method), | ||
| } | ||
| for class_ast in self.ast.subtrees(ASTNodeType.CLASS_DECLARATION) | ||
| for class_declaration in [class_ast.root()] | ||
| for method in class_declaration.methods | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the `methods` computed field definition used by ASTNode for CLASS_DECLARATION
rg -n "'methods'" aibolit/ast_framework -C5
ast-grep run --pattern 'CLASS_DECLARATION: {$$$}' --lang python aibolit/ast_frameworkRepository: cqfn/aibolit
Length of output: 2309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the AST/computed-fields implementation and existing NPath tests.
git ls-files 'aibolit/ast_framework/**' 'aibolit/metrics/npath/**' 'tests/**' | sed -n '1,200p'
echo '--- standard_fields.py ---'
cat -n aibolit/ast_framework/computed_fields_catalog/standard_fields.py | sed -n '1,140p'
echo '--- AST subtree implementation ---'
rg -n "def subtrees|class AST|subtrees\(" aibolit/ast_framework -C 3
echo '--- NPath metric and tests ---'
rg -n "class TestMvnFreeNPathMetric|NPath|inner class|nested class|class .*class" aibolit/metrics/npath tests -C 3Repository: cqfn/aibolit
Length of output: 10798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- AST.subtrees implementation ---'
cat -n aibolit/ast_framework/ast.py | sed -n '60,130p'
echo '--- ASTNodeType and class declaration handling ---'
cat -n aibolit/ast_framework/ast_node_type.py | sed -n '1,120p'
cat -n aibolit/ast_framework/ast_node.py | sed -n '1,220p'
echo '--- NPathMetric / MvnFreeNPathMetric around report() ---'
cat -n aibolit/metrics/npath/main.py | sed -n '100,150p'
echo '--- tests mentioning MvnFreeNPathMetric or inner classes ---'
rg -n "MvnFreeNPathMetric|TestMvnFreeNPathMetric|inner class|nested class|class .*class" aibolit tests -C 3Repository: cqfn/aibolit
Length of output: 20529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- nodes_filter_factory ---'
cat -n aibolit/ast_framework/computed_fields_catalog/nodes_filter.py | sed -n '1,220p'
echo '--- any tests / fixtures for nested classes in the repo ---'
rg -n "nested class|inner class|CLASS_DECLARATION" aibolit -g '*test*' -g '*Test*' -C 2
echo '--- references to class_declaration.methods ---'
rg -n "\.methods\b" aibolit -C 2Repository: cqfn/aibolit
Length of output: 1459
Nested class methods are skipped here. AST.subtrees(ASTNodeType.CLASS_DECLARATION) only returns the outer class subtree, and methods only filters direct METHOD_DECLARATION children from each class body. Any method declared inside an inner class is omitted from both report() and value(). Add a test for a nested class to lock the intended behavior in.
🤖 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 `@aibolit/metrics/npath/main.py` around lines 110 - 122, The report generation
in report() and the aggregate in value() are skipping methods declared inside
nested classes because AST.subtrees(ASTNodeType.CLASS_DECLARATION) and
class_declaration.methods only cover direct class methods. Update the traversal
in NPathCalculator so nested class methods are either included intentionally or
explicitly excluded with the correct behavior, and add a regression test
covering an inner class to verify the expected result.
| excluded_metric_codes = set(patterns_config['metrics_exclude']) | ||
| ast_compatible_metrics = [ | ||
| metric_config | ||
| for metric_config in patterns_config['metrics'] | ||
| if metric_config['code'] not in excluded_metric_codes | ||
| ] | ||
| for metric_config in ast_compatible_metrics: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect config.py context around metrics_exclude and the metric classes it lists
rg -n -B5 -A2 "metrics_exclude" aibolit/config.pyRepository: cqfn/aibolit
Length of output: 652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== test/config/test_config.py ==\n'
cat -n test/config/test_config.py | sed -n '1,220p'
printf '\n== aibolit/config.py ==\n'
cat -n aibolit/config.py | sed -n '1,320p'
printf '\n== Search for metrics_exclude usage ==\n'
rg -n "metrics_exclude|ast: AST|AST-annotated|accepts an ast" -S aibolit testRepository: cqfn/aibolit
Length of output: 26724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Excluded metric implementations ==\n'
for f in \
aibolit/metrics/entropy/entropy.py \
aibolit/metrics/spaces/SpaceCounter.py \
aibolit/metrics/lcom4/lcom4.py \
aibolit/metrics/number_variables/numVariables.py \
aibolit/metrics/NumberMethods/NumberMethods.py
do
printf '\n-- %s --\n' "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: cqfn/aibolit
Length of output: 6155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== __main__.py relevant sections ==\n'
cat -n aibolit/__main__.py | sed -n '260,360p'
printf '\n== ml_pipeline.py relevant section ==\n'
cat -n aibolit/ml_pipeline/ml_pipeline.py | sed -n '60,120p'
printf '\n== model.py relevant section ==\n'
cat -n aibolit/model/model.py | sed -n '1,120p'Repository: cqfn/aibolit
Length of output: 12816
Filter the test by AST compatibility, not metrics_exclude. metrics_exclude is used for pipeline/model feature selection and already includes M8 (NumberMethods), which does accept ast: AST. This test will skip valid metrics and can miss regressions; iterate over all metrics or keep a separate AST-incompatible list instead.
🤖 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 test setup is
incorrectly filtering metrics using metrics_exclude, which can hide valid
AST-compatible metrics like NumberMethods/M8. Update the logic in the
test_config setup to select metrics based on AST compatibility instead of
reusing metrics_exclude, and reference the existing ast_compatible_metrics loop
in test_config.py to either iterate over all metrics or maintain a separate list
of AST-incompatible metrics.
| def testIncorrectFormat(): | ||
| with pytest.raises(Exception, match='PMDException'): | ||
| file = 'test/metrics/npath/ooo.java' | ||
| metric = NPathMetric(file) | ||
| metric.value(True) | ||
| file = 'test/metrics/npath/ooo.java' | ||
| metric = NPathMetric(file) | ||
| if shutil.which('mvn') is None: | ||
| with pytest.raises(Exception): | ||
| metric.value(True) | ||
| else: | ||
| with pytest.raises(Exception, match='PMDException'): | ||
| metric.value(True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Blind Exception assertion flagged by static analysis (B017).
Since _value_without_maven() (main.py) also raises a plain Exception with a known message ('File ... does not exist'), the no-mvn branch could match on that message the same way the mvn branch matches 'PMDException', tightening the assertion.
🧪 Suggested tightening
if shutil.which('mvn') is None:
- with pytest.raises(Exception):
+ with pytest.raises(Exception, match='does not exist'):
metric.value(True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def testIncorrectFormat(): | |
| with pytest.raises(Exception, match='PMDException'): | |
| file = 'test/metrics/npath/ooo.java' | |
| metric = NPathMetric(file) | |
| metric.value(True) | |
| file = 'test/metrics/npath/ooo.java' | |
| metric = NPathMetric(file) | |
| if shutil.which('mvn') is None: | |
| with pytest.raises(Exception): | |
| metric.value(True) | |
| else: | |
| with pytest.raises(Exception, match='PMDException'): | |
| metric.value(True) | |
| def testIncorrectFormat(): | |
| file = 'test/metrics/npath/ooo.java' | |
| metric = NPathMetric(file) | |
| if shutil.which('mvn') is None: | |
| with pytest.raises(Exception, match='does not exist'): | |
| metric.value(True) | |
| else: | |
| with pytest.raises(Exception, match='PMDException'): | |
| metric.value(True) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 20-20: Do not assert blind exception: Exception
(B017)
🤖 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/metrics/npath/test_all_types.py` around lines 16 - 24, The no-mvn branch
in testIncorrectFormat uses a blanket Exception assertion, which is too loose.
Tighten the pytest.raises call to match the specific message raised by
NPathMetric.value/_value_without_maven, similar to how the mvn branch matches
PMDException. Use the known “File ... does not exist” message pattern so the
test verifies the intended failure path without accepting unrelated exceptions.
Source: Linters/SAST tools
|
@Beeeeeeee-c conflicts here |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests