Generic: fix unintentional errors in test case files #2105
Conversation
At times, parse errors in unit test case files are intentional to test that a sniff handles these correctly. However, _unintentional_ parse errors or other code errors which would cause fatal errors, which were not introduced to specifically test the sniff, do not need to be in these files.
…sibility global functions Global functions are always public and cannot be qualified with a visibility keyword.
The unit tests for this sniff tested properties with the `final` keyword, however, that is not a valid syntax. The `final` keyword can only be applied to classes and methods. These changes, make the unit tests more realistic. The sniff itself reports on finding the `final` keyword within a `final` class independently of the context in which the keyword is used, so did not need any changes.
A number of these test cases are still parse errors as an array item cannot be a property - i.e. `$array[$test]` -, but it is better than it was.
The exact same test can be found slightly further up in each block, so this doesn't actually test anything new.
Magic methods without the required arguments are compile errors.
The code `$x = 'My '.1234;` is a parse error as the `.` would tokenize together with the number to a `T_DNUMBER` and not to a `T_STRING_CONCAT`, which means nothing was actually being tested. By adding the space, the `.` is now correctly tokenized which activates the test that this _doesn't_ give an error properly.
jrfnl
force-pushed
the
feature/generic-fix-unintentional-errors-test-files
branch
from
8989ac6 to
69eebff
Compare
| @@ -77,4 +77,4 @@ function bar($x) | |||
|
|
|||
| function ($a, $b) { | |||
| return $a * 2; | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
Both this one as well as the one below are closures. If the ; is missing after the closing brace, it is considered a parse error by PHP.
There was a problem hiding this comment.
/facepalm I completely missed the lack of function name in there.
| const PRIVATE; | ||
| HttpStatus::CONTINUE; | ||
| Function ($f) { | ||
| Yield $f; | ||
| Yield From fun(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This one might have been better fixed with $foo = Function...
There was a problem hiding this comment.
That wouldn't fix the parse error.
There was a problem hiding this comment.
Sorry, I meant assigning to a var as well as the semicolon on the end - just to make it look somewhat closer to real code. That's what tripped me up with the one above.
But honestly, it's not big deal. I just started reviewing stuff because I had a few spare minutes and realised I got through a single commit before I ran out of time.
There was a problem hiding this comment.
Well, you picked the biggest commit, so you've reviewed 70% already. The other ones are much smaller so should be easy going.
I'll assign those closures to a var, as - while not having this does not cause parse errors, I do agree that it makes for more realistic code.
There was a problem hiding this comment.
@gsherwood I've added a new commit which addresses this for all such test code involving closures for the Generic sniffs.
Verified that all closures in test cases are: * either assigned to a variable * or used as a function call parameter / callback * or otherwise "valid"/realistic code In those cases where this wasn't the case before, the test cases have been adjusted in the most minimal fashion possible.
[Fixer conflicts series PR]
While looking through the remaining fixer conflicts, I did a quick check to see if any of them may be caused by parse or compile errors in the test case files. Hint: Yes, some are.
In my opinion, sniffs should be resilient against parse errors caused by live coding so they can be used in an IDE, but do not need to account for every single parse/compile error possible which could prevent it from working.
Some inaccurate results when a sniff is run on a file with a parse/compile error should be expected and acceptable.
So to this end, I've linted the test case files and fixed any parse errors and compile errors in these which were not explicitly testing for the handling of parse errors by the sniff.
I've chosen to err on the side of caution, only fixing parse/compile errors which were clearly not testing anything in the sniff and left parse/compile errors in when in doubt.
This is the part of a series of PRs doing this for the test case files in each standard.
Most parse errors were of the following types:
class ABC() {}.Compile errors were mostly:
Duplicate function names used.Related commit removed as per PEAR: fix unintentional errors in test case files #2106 (comment)This PR will be easiest to review by looking at the individual commits in combination with the more detailed commit messages.
This PR also contains a few QA improvements to some test case files where the unit tests weren't actually testing anything (new) or only testing invalid code. These kind of improvements are each in separate commits.