Generic/AbstractClassNamePrefix: improve code coverage #641
Conversation
There was a problem hiding this comment.
Hi @rodrigoprimo, all good in principle, but can do with some fine-tuning still. See inline comments.
I'm also wondering if, while the test file is being touched anyway, some minor cleanup should be done ?
// error=>// Error.(multiple times)$var = 'abstract class IncorrectNameButOk';=>$var = 'abstract class TextStringsAreDisregarded';(test isn't really testing anything, but I can see why it is there, so let's leave it in, but make it more descriptive)abstract class abstractOkName=>abstract class abstractOkCaseOfPrefixIsNotEnforcedclass NameAbstractBar {}=>class NotAnAbstractClassSoNoPrefixRequired {}
Also the $abstractVar = ''; and $abstracVar = ''; tests aren't testing anything, so could be removed.
src/Standards/Generic/Tests/NamingConventions/AbstractClassNamePrefixUnitTest.1.inc
Show resolved
Hide resolved
src/Standards/Generic/Tests/NamingConventions/AbstractClassNamePrefixUnitTest.1.inc
Outdated
Show resolved
Hide resolved
|
Thanks for your review, @jrfnl. I replied to the inline comments and implemented the additional cleanup that you suggested. When you get a chance, could you please check this PR again? |
There was a problem hiding this comment.
Thanks @rodrigoprimo ! Looking good.
I'm going to rebase the PR and squash a few commits before merging and I'll make a few more small tweaks to the tests to clean up even further.
|
@rodrigoprimo Actually, before I rebase & squash, would you like me to add a separate commit to show the additional tweaks I want to make ? [Edit]: just give me a sign once you've had a chance to look at the additional changes. I'll merge after that. |
jrfnl
force-pushed
the
test-coverage-abstract-class-name-prefix
branch
from
82bb018
to
787bd71
Compare
|
@jrfnl, I just checked the commits you added, and they look good to me. Thanks! |
Doing this to be able to create tests with syntax errors in additional, separate test case files.
This sniff only listens to `T_CLASS`. It does not listen to `T_ANON_CLASS`. So the removed code comment is incorrect. The if condition is still valid to bail early when live coding, but it does not ever apply to anonymous classes.
…ests Test improvements: * Adjust some existing tests to have comments and new lines in unexpected places. * Adjust some existing tests to include extended classes and implemented interfaces. * Add tests with `final` and `readonly` class modifier keywords. * Make some tests more descriptive by making the class name reflect what is being tested. * Add a few comments to pre-existing tests to clarify why they exist. * Add a separate test case file with a live coding/parse error test. * Remove some code snippets which weren't testing anything. Clean up the test file: * Remove redundant whitespace in the test case file. * Use proper punctuation in comments in test case file.
jrfnl
force-pushed
the
test-coverage-abstract-class-name-prefix
branch
from
787bd71
to
b80228e
Compare
|
Rebased & cleaned up the commits. Merging once the build has passed. |
Description
This PR improves code coverage for the
Generic.NamingConventions.AbstractClassNamePrefixsniff and removes an incorrect code comment.Related issues/external references
Part of #146
Types of changes
PR checklist