Generic/LanguageConstructSpacing: improve sniff code coverage #176
Conversation
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.
There was a problem hiding this comment.
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).
|
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 |
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. |
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 ifcondition 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
PR checklist