PEAR: fix unintentional errors in test case files #2106
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Methods in an interface must be public.
Magic methods without the required arguments are parse errors.
Properties in a class need to have either visibility of a `var` prefix/modifier.
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.
…ility global functions Global functions are always public and cannot be qualified with a visibility keyword
Don't make me use unique function names in test classes... please :) |
I figured that might be contentious, which is why it was in a separate commit. 😎 I'll remove that particular commit from both this PR as well as the other one. |
jrfnl
force-pushed
the
feature/pear-fix-unintentional-errors-test-files
branch
from
46ff7e5 to
016d870
Compare
|
FYI: I've removed the commit from both PRs & force pushed the branches, so should be good, or at least, better now ;-) |
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.
jrfnl
force-pushed
the
feature/pear-fix-unintentional-errors-test-files
branch
from
742e5dd to
4b1127e
Compare
|
This, and the Generic PR, were huge efforts. Thanks a lot for cleaning up these test files. |
|
You're welcome. Will try to do the same for the PSR1/2/12/Squiz and Zend standards in the near future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[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.