Generic/UselessOverridingMethod: improve code coverage

[rodrigoprimo]

Description

This PR improves code coverage for the Generic.CodeAnalysis.UselessOverridingMethod sniff.

Related issues/external references

Part of #146

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo Thanks for working on this. My apologies it took me a while before reviewing this PR.

This PR review is a mix of a review of the changes you've made in combination with a critical look at the sniff and what (other) tests are missing, which brings to light some bugs.

So after reviewing, these are my findings:

1. I believe the "skip function without body" block is now covered via a parse error code sample in test case file 2.
   What about the following non-parse error situations, which would also hit that code block and are currently not covered by tests ?
   • an abstract method declaration.
   • an interface method declaration.
2. The "Any token except 'parent'" block is now covered via the ignoreNoParent() code snippet.
   What about adding an additional test with valid code, but without a return statement ? (void method)
3. The "Skip for ... other method." block is now covered via the differentParentMethod() code snippet.
   I can see a bug in the condition though - function/method names in PHP are case-insensitive.
   There is currently no test where the function declaration name is not in the same case as the name used in the function call.
   Such a test should be added and the code should be flagged as potentially useless override, which would need a small fix in the condition of that code block.
   This would be a functional change and could be made in a separate commit in this PR or via a separate PR.
4. I'm missing a test where the function call has an open parenthesis, but no close parenthesis.
   Adding this test would highlight that some extra defensive coding would be helpful.
   The "Skip for invalid code." block, which checks for the open parenthesis, should probably also check for a matching close parenthesis via isset($tokens[$next]['parenthesis_closer']) !== false.
   The redundant function call on line 118 - count($tokens) - can then be also removed and the $count in the for condition replaced. (That function call was redundant anyhow and should have used $phpcsFile->numTokens instead)
   This would be a functional change and could be made in a separate commit in this PR or via a separate PR.
5. I'm missing a test with a call to parent::methodName() and something else between that and the semicolon.
   Something like: return parent::methodName($a, $b) + $b;
6. I'm missing a test with a function call with a trailing comma as allowed since PHP 7.3.
   Looks like the sniff would handle this correctly due to the array_filter(), but it should still be tested.
7. I'm missing a test with a call to parent::methodName() using the same parameters as in the method signature, but in a different order.
   Looks like the sniff would handle this correctly, but I think it should still be covered by a test.
8. I'm missing a test with a call to parent::methodName() with the same number of parameters, but different names.
   Looks like the sniff would handle this correctly, but I think it should still be covered by a test.
   Something like return parent::methodName($this->prop[$a], $this->{$b}); maybe ? (to make it valid code)
9. Looking critically, the $next === false condition seems redundant in nearly all conditions (and there are no tests conceivable which could ever hit the condition).
   If the function declaration would not have a scope closer, the scope opener would not be set, so $next can never be false as there will always be a close curly after whatever code we're looking at.
   Having said that, if the $next === false bits would be removed, it might be prudent to update the "Skip function without body." block to also verify there is a scope closer.
10. I'm missing a test with non-OO function declaration. Use of the parent keyword in the body of such a function would be invalid no matter what, however, for the purposes of this sniff, I think it would be more appropriate to ignore such functions as they are, for sure, not a "method override".
    What do you think ? This would be a functional change and could be made in a separate commit in this PR or via a separate PR.

The test-only changes should be included in this PR. As for the rest, it's up to you to decide whether you want to work on those changes too. If not, we can open an issue for the other points to be addressed separately at a later point in time.

[rodrigoprimo]

Thank you very much for the detailed review, @jrfnl!

I pushed a commit adding the new tests that you suggested. I will create a separate PR for the functional changes.

What about the following non-parse error situations, which would also hit that code block and are currently not covered by tests?

an abstract method declaration.
an interface method declaration.

I also added a trait method declaration.

This PR is ready for another review.

[jrfnl]

I will create a separate PR for the functional changes.

Excellent! In the mean time, should we open an issue about those other observations to make sure they don't get lost ? (as this PR will hopefully be merged & closed after the next review)

[jrfnl]

@rodrigoprimo Thanks for the update. All good.

I'll rebase (without changes) and will merge this once the build has passed.

For the record, of the list from my previous review, the following points still need addressing in a follow-up PR: 3, 4, 9 and 10.

[rodrigoprimo]

Thanks for merging this PR.

In the mean time, should we open an issue about those other observations to make sure they don't get lost ?

I just opened a PR addressing all the remaining points that you raised: #366