Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic/DisallowYodaConditions: improve code coverage #465

Merged

Conversation

rodrigoprimo
Copy link
Contributor

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 (returning true if the passed token index is not an array):

I left a comment documenting that the line cannot be covered by tests but cannot be removed as well.

I considered using // @codeCoverageIgnore or // @codeCoverageIgnoreStart and // @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.Found and Squiz.Commenting.InlineComment.InvalidEndChar). I will open a separate issue for this.

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

* 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 jrfnl force-pushed the test-coverage-disallow-yoda-condition branch from adcaad4 to 48a1a43 Compare May 13, 2024 16:12
@jrfnl
Copy link
Member

jrfnl commented 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.

@jrfnl jrfnl merged commit c336428 into PHPCSStandards:master May 13, 2024
40 checks passed
@rodrigoprimo rodrigoprimo deleted the test-coverage-disallow-yoda-condition branch May 13, 2024 19:42
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants