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/LanguageConstructSpacing: improve sniff code coverage #176

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Dec 20, 2023

Description

This PR improves the code coverage for the Generic/LanguageConstructSpacing sniff.

There was just one block of code that was not covered by tests: /src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php#L131. It is an else if condition to handle language constructs that are followed by some content without a space or opening parenthesis first. This adds a few examples to make sure this part of the code is covered.

This PR also restructures the test case files for this sniff and moves an intentional parse error test to its own file per #143.

Code coverage for the Generic/ArbitraryParenthesesSpacing sniff before this PR:

https://coveralls.io/builds/64633852/source?filename=src%2FStandards%2FGeneric%2FSniffs%2FWhiteSpace%2FLanguageConstructSpacingSniff.php

And after this PR:

https://coveralls.io/builds/64687021/source?filename=src%2FStandards%2FGeneric%2FSniffs%2FWhiteSpace%2FLanguageConstructSpacingSniff.php

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.

Doing this to be able to move tests with syntax errors to their own
file.
This sniff has an `else if` condition
(https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/c855348766a424d23425eba5cc4420e26e4a3e38/src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php#L131)
to handle language constructs that are followed by some content without
a space or opening parenthesis first. This code was not exercised by the
automated tests.

This commit adds a few examples to make sure this part of the code is
covered.
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.

Thanks for this PR @rodrigoprimo.

Please keep this general rule of thumb in mind for future PRs: ALWAYS put new tests at the end of a test case file.

This avoids having to change the line numbers in the data provider array and keeps the diff confined to what has actually changed. Changing the line numbers in the array makes it much much harder for a reviewer to see if any expectations for pre-existing tests may have accidentally or deliberately changed.

Note: in this case, the renumbering could have been largely avoided anyway, even while putting the tests where they are now, if you'd have used the blank lines in between the test "sets" instead of adding a new line for these.

I'll accept this PR as I've done the (painful) review now anyway, but this is a one-time only exception and I will not accept PRs which change the test array for pre-existing tests in the future (unless there is a very good reason for it).

@jrfnl jrfnl merged commit f436108 into PHPCSStandards:master Dec 24, 2023
42 checks passed
@rodrigoprimo
Copy link
Contributor Author

rodrigoprimo commented Jan 2, 2024

Apologies for the extra work I gave you when reviewing this PR, @jrfnl. I had wrongly assumed that it was preferable to group related tests together instead of adding them at the end of the file. I will make sure not to do this again.

What do you think about adding a note to CONTRIBUTING.md saying that new tests should be added to the end of the test case file? If you believe that is a good idea, I can create a PR.

@rodrigoprimo rodrigoprimo deleted the language-construct-spacing-improve-code-coverage branch January 2, 2024 14:57
@jrfnl
Copy link
Member

jrfnl commented Jan 2, 2024

What do you think about adding a note to CONTRIBUTING.md saying that new tests should be added to the end of the test case file? If you believe that is a good idea, I can create a PR.

I can't remember the last time I had to tell the above to a contributor, or if I've ever had to point this out at all, so I'm not sure it needs to be spelled out in the CONTRIBUTING as it seems to be obvious to most everyone else.

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