-
Notifications
You must be signed in to change notification settings - Fork 52
Extend MvnFreeNPathMetric to handle while loops
#861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8808d27
9fe30c5
b8b47a3
9f460b1
c3fa198
04705ba
f50f138
fc02d4b
4c4c742
e1f676f
3074938
f7e325b
b243cd3
caf4860
2f67c86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,15 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import os | ||
| import pathlib | ||
| from textwrap import dedent | ||
|
|
||
| import pytest | ||
|
|
||
| from aibolit.ast_framework import AST | ||
| from aibolit.metrics.npath.main import MvnFreeNPathMetric, NPathMetric | ||
| from aibolit.utils.ast_builder import build_ast_from_string | ||
| from aibolit.utils.ast_builder import build_ast, build_ast_from_string | ||
|
|
||
|
|
||
| def testIncorrectFormat(): | ||
|
|
@@ -433,9 +435,174 @@ class Test { | |
| ''').strip() | ||
| assert self._value(content) == 5 | ||
|
|
||
| def test_simple_while_loops(self) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ivanovmg The tests are clear and sufficient! I would also suggest making sure that the tests from the previous implementation work for the new implementation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AntonProkopyev the tests for the existing case files do not actually pass for |
||
| content = dedent(''' | ||
| class Test { | ||
| void simpleWhile() { | ||
| int i = 0; | ||
| while (i < 10) { | ||
| i++; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 2 | ||
|
|
||
| def test_while_with_if(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| void whileWithIf(int x) { | ||
| while (x > 0) { | ||
| if (x % 2 == 0) { | ||
| System.out.println("Even"); | ||
| } | ||
| x--; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 3 | ||
|
|
||
| def test_while_with_or_condition(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| void whileWithOrCondition(int x, int y) { | ||
| while (x > 0 || y > 0) { | ||
| if (x > 0) { | ||
| System.out.println("X is positive"); | ||
| } | ||
| if (y > 0) { | ||
| System.out.println("Y is positive"); | ||
| } | ||
| x--; | ||
| y--; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 6 | ||
|
|
||
| def test_while_with_and_condition(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| void whileWithAndCondition(int x, int y) { | ||
| while (x > 0 && y > 0) { | ||
| System.out.println("Both X and Y are positive"); | ||
| x--; | ||
| y--; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 3 | ||
|
|
||
| def test_while_with_break(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| public void loopWithBreak() { | ||
| int i = 0; | ||
| while (i < 10) { | ||
| if (i == 5) { | ||
| break; | ||
| } | ||
| i++; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 3 | ||
|
|
||
| def test_empty_while_loop(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| public void emptyLoop() { | ||
| while (condition) {} | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 2 | ||
|
|
||
| def test_nested_while_loops(self) -> None: | ||
| content = dedent(''' | ||
| public class Test { | ||
| public void nestedLoops() { | ||
| int i = 0; | ||
| while (i < 3) { | ||
| int j = 0; | ||
| while (j < 2) { | ||
| j++; | ||
| } | ||
| i++; | ||
| } | ||
| } | ||
| } | ||
| ''').strip() | ||
| assert self._value(content) == 3 | ||
|
|
||
| @pytest.mark.xfail( | ||
| strict=True, | ||
| reason='Incorrect implementation. Most likely need to review if statement', | ||
| ) | ||
| def test_complicated(self) -> None: | ||
| # @todo #852:60min Fix MvnFreeNPathMetric for moderate NPath complexity case | ||
| # It is necessary to fix implementation of MvnFreeNPathMetric | ||
| # so that the test on a moderate NPath complexity case passes. | ||
| # Once fixed, remove `pytest.mark.xfail` decorator. | ||
| # | ||
| # It is most likely that the implementation of if statements is wrong. | ||
| # Refer to https://checkstyle.org/checks/metrics/npathcomplexity.html, | ||
| # where it is stated that complexity for the `if` statement is a sum | ||
| # of NPath complexities for the condition, then and else parts. | ||
| # On the other hand when following this calculation, the simple if-else statement | ||
| # would have complexity of 3 rather than 2. | ||
| assert self._value_from_filepath(self._filepath('Complicated.java')) == 12 | ||
|
|
||
| @pytest.mark.xfail( | ||
| strict=True, | ||
| reason='Incorrect implementation. Most likely need to review if/switch statements', | ||
| ) | ||
| def test_even_more_complicated(self) -> None: | ||
| # @todo #852:60min Fix MvnFreeNPathMetric for high NPath complexity case | ||
| # It is necessary to fix implementation of MvnFreeNPathMetric, | ||
| # so that the test on a very high NPath complexity case passes. | ||
| # Once fixed, remove `pytest.mark.xfail` decorator. | ||
| # | ||
| # Although the comment in Foo.java states that the expected value is 200, | ||
| # the actual value, verified against `checkstyle-10.26.1-all.jar` is 288. | ||
| # | ||
| # $ cat config.xml | ||
| # <?xml version="1.0"?> | ||
| # <!DOCTYPE module PUBLIC | ||
| # "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" | ||
| # "https://checkstyle.org/dtds/configuration_1_3.dtd"> | ||
| # <module name="Checker"> | ||
| # <module name="TreeWalker"> | ||
| # <module name="NPathComplexity"> | ||
| # <property name="max" value="1"/> | ||
| # </module> | ||
| # </module> | ||
| # </module> | ||
| # | ||
| # $ java -jar checkstyle-10.26.1-all.jar -c config.xml test/metrics/npath/Foo.java | ||
| # [ERROR] ... NPath Complexity is 288 (max allowed is 1). [NPathComplexity] | ||
| assert self._value_from_filepath(self._filepath('Foo.java')) == 288 | ||
|
|
||
| def _filepath(self, basename: str) -> pathlib.Path: | ||
| return pathlib.Path(__file__).parent / basename | ||
|
|
||
| def _value(self, content: str) -> int: | ||
| return MvnFreeNPathMetric( | ||
| return self._metric( | ||
| AST.build_from_javalang( | ||
| build_ast_from_string(content), | ||
| ), | ||
| ).value() | ||
| ) | ||
|
|
||
| def _value_from_filepath(self, filepath: str | os.PathLike) -> int: | ||
| return self._metric( | ||
| AST.build_from_javalang( | ||
| build_ast(filepath), | ||
| ), | ||
| ) | ||
|
|
||
| def _metric(self, ast: AST) -> int: | ||
| return MvnFreeNPathMetric(ast).value() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I had to perform refactoring towards this dictionary dispatch pattern to avoid too many returns.
In particular, the linter was complaining about 7 return statements with a max of 6.
We can consider reverting and silencing the warning if the original implementation is considered more readable.
Alternatively, we can simply write resulting value to a variable
resultand then only returnresultonce.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanovmg IMO the dictionary dispatching is pretty clean and easy to read. Maybe we could consider add explicit
ifwhen default handle is needed but thelambdais also OK for me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AntonProkopyev I extracted a separate function
default_handler. I hope it helps convey the message better than an anonymous function. Does it look OK?