Generic/DisallowYodaConditions: improve code coverage #465
Conversation
This file contains 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
jrfnl
reviewed
May 2, 2024
There was a problem hiding this comment.
@rodrigoprimo Thanks for this PR. I've left a few small comments inline to have a look at, but generally speaking, this is looking all good.
I've not done a detailed test vs sniff review as this is known to be a sniff with numerous bugs still to be fixed, so most issues are probably already known and/or will be found when addressing the known issues.
Not something which should hold back this PR.
src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php
Show resolved
Hide resolved
src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php
Outdated
Show resolved
Hide resolved
src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php
Outdated
Show resolved
Hide resolved
* Remove unused variable. * Fix typo in code comment. * Remove incorrect code comment.
Includes removing two unreachable conditions The first condition is unreachable because PHPCS will never call DisallowYodaConditionsSniff::process() if there is no non-empty token before a comparison token. Even if the file contains a syntax error, there must be at least a PHP opening tag before the comparison token for the method to be called. The second condition is unreachable because at this point in the code there will always be at least two non-empty tokens before the comparison token. If there is only one, it must be a PHP opening tag and, in this case, the method will bail before reaching the code that sets the `$prevIndex` variable. This commit also documents why a certain line is uncovered by tests - and cannot be covered, nor can it be removed as external sniffs may call `DisallowYodaConditionsSniff::isArrayStatic()` directly.
This commit renames a variable to use a better name that actually matches what the variable contains. The previous name was misleading as the variable did not contain the index of the closing parenthesis.
jrfnl
force-pushed
the
test-coverage-disallow-yoda-condition
branch
from
adcaad4
to
48a1a43
Compare
jrfnl
approved these changes
May 13, 2024
|
I've rebased the PR without changes (other than re-organizing the commits, but no changes to the code). Merging once the build has passed. |
rodrigoprimo
added a commit
to rodrigoprimo/PHP_CodeSniffer
that referenced
this pull request
Jun 7, 2024
This commit adds the `@codeCoverageIgnoreStart/@codeCoverageIgnoreEnd` PHPUnit annotations to ignore, for the purposes of code coverage, a line that can't be tested. I considered doing this initially when I worked on improving code coverage for this sniff in PHPCSStandards#465, but I wrongly assumed that was not an option due to the `Squiz.Commenting.InlineComment.InvalidEndChar` error (which is part of the standard used by PHPCS). What I failed to notice back then is that I was adding the annotation below a comment, and this triggers the error: ``` // Shouldn't be possible but may happen if external sniffs are using this method. // @codeCoverageIgnoreStart ``` Adding the annotation before the comment as it is done in this commit does not trigger the error because the `Squiz.Commenting.InlineComment` sniff will only trigger the `InvalidEndChar` error if the first character of the comment is a letter. Using `return true; // @codeCoverageIgnore` is not an option because it triggers `Squiz.Commenting.PostStatementComment.Found`.
11 tasks
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.
Description
This PR improves code coverage for the Generic.ControlStructures.DisallowYodaConditions sniff. It adds a few tests for uncovered lines and documents using tests a few comparison operators that this sniff listens for that were not part of the tests before.
Besides that, this PR removes an unused variable, fixes a typo in a code comment, removes two unreachable variables, and renames a variable to make it more clear what it contains.
It is impossible to add code coverage for the line below using the current test framework and we should not remove it as external sniffs could be using the
DisallowYodaConditionsSniff::isArrayStatic()and relying on the current behavior (returningtrueif the passed token index is not an array):PHP_CodeSniffer/src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php
Line 152 in 30b3148
I left a comment documenting that the line cannot be covered by tests but cannot be removed as well.
I considered using
// @codeCoverageIgnoreor// @codeCoverageIgnoreStartand// @codeCoverageIgnoreEnd, but that is not possible as well as those annotations trigger different sniff errors for the coding standard used by PHPCS (Squiz.Commenting.PostStatementComment.FoundandSquiz.Commenting.InlineComment.InvalidEndChar). I will open a separate issue for this.Related issues/external references
Part of #146
Types of changes
PR checklist