Generic/CyclomaticComplexity: improve code coverage #684
Conversation
rodrigoprimo
force-pushed
the
test-coverage-cyclomatic-complexity
branch
from
1042b40 to
da0f04b
Compare
Please move these questions/points to individual issues. They each have their own decision point and those decision points are unrelated to the decision point of this PR. |
There was a problem hiding this comment.
@rodrigoprimo Thanks for this PR and for continuing with the work to increase code coverage.
IMO this one needs a little more work.
Aside from the comments left in-line, I'd like to ask you to have a look at the list of tokens the sniff is looking for:
I can see at least four tokens in that list which are not used in the tests. I believe tests should be added/adjusted to ensure those tokens are included in the tests.
Aside from that, on top of the questions you already asked (and which need to be moved to issues), I also saw another issue in this sniff for which I've opened issue #726.
src/Standards/Generic/Sniffs/Metrics/CyclomaticComplexitySniff.php
Outdated
Show resolved
Hide resolved
|
Thanks for your review, @jrfnl. I just replied to your inline comments.
Good point. I updated the existing tests to include at least one occurrence of each of the tokens that the sniff looks for to count cyclomatic complexity. I added the following tokens: T_ELSEIF, T_CATCH, T_FOREACH and T_FOR. |
…` is not set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (PHPCSStandards#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution.
Doing this to be able to create tests with syntax errors on separate files.
There was a problem hiding this comment.
Thanks again for this @rodrigoprimo !
I'll rebase the PR and squash most of the commits. Will merge once the build has passed again.
…` is not set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (PHPCSStandards#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution.
jrfnl
force-pushed
the
test-coverage-cyclomatic-complexity
branch
from
8b26262 to
54a05bc
Compare
…` is not set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution.
… set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (PHPCSStandards#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution. This commit also updates the related code comment to better reflect what the if condition does.
… set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (PHPCSStandards#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution. This commit also updates the related code comment to better reflect what the if condition does.
… set Quoting @jrfnl: "Only checking for the scope_opener, when accessing/using both the scope_opener and scope_closer indexes, is probably fine in practical terms. However, the part of the code base which sets these indexes is not sufficiently covered by tests, nor does it document that those indexes will only be set if both can be set, so there may be edge case exceptions" (#684 (comment)). The sniff was already working fine before this change. Checking if `scope_closer` is just an extra precaution to err on the side of caution. This commit also updates the related code comment to better reflect what the if condition does.
Description
This PR improves code coverage for the
Generic.Metrics.CyclomaticComplexitysniff.I noticed three things while working on this sniff that we might want to address in the future:
defaultstatement, while PHPMD/PHP Depend doesn't (https://pdepend.org/documentation/software-metrics/cyclomatic-complexity.html).Just looking for some initial feedback on those three items to decide if I should address them or not on future PRs. I am happy to move them to an issue as well if you prefer. I'm inclined to think that the first two items are worthwhile and the third is not (assuming this is not a very popular sniff and that there is no easy-to-find clear definition of how to count cyclomatic complexity in PHP).
Related issues/external references
Part of #146
Types of changes
PR checklist