Are you curious about what I do at Etsy as an Anthropologist of Developer Culture? Check out the most recent Voices of the ElePHPant interview with me, where I answer this question and a little more.
Thoughts on Accessibility with Regards to Testing
In Alternative Thoughts on Sniffs for Useless Method Overriding I discussed the discovery of a horrible dance that some developers will do just to test private functions:
- See a
privatemethod you want to test in isolation - Upgrade the
privatetoprotected - Create a child class implementation of this class in the
TestCasefile - Override any
protectedmethods, that were actually intended to beprivatemethods and make them nowpublic - Test the new child class instead of the actual class
This violates the only one class per test file rule, as discussed in PHPUnit Coding Standard: Classes
Many claim that they do this because they do not want the methods to be part of the public API, but need to make it protected so they can do this hack. Yet those who argue for this so called solution have nothing to reconcile the fact that they have removed the ability to distinguish between methods that should be override-able by child implementations and those that should not.
In all honesty, if you have functionality that you would prefer not to be in the public API of one object, but it is sufficiently complex to warrant it’s own battery of tests, you should take that as a suggestion to pull that functionality out into it’s own object.
It is a pity to hide something so complex in a private function, and tempt others into exercising copy/paste coding practices to use it.
If you are using PHP 5.3, then you can do another cheat that will let you avoid the number of class violations, and let you keep using the private keyword. Sebastian Bergmann discusses this in Testing Your Privates.
Don’t jump the gun. This is still not license to blindly make things private. Even Sebastian repeats in the post and in the comments:
Just because the testing of protected and private attributes and methods is possible does not mean that this is a “good thing”.
So why am I even talking about these naughty ways to test private methods?
Because I cannot necessarily stop you!
So let me offer an alternative to all of this that should help communicate intent in your code, let you use all the accessibility levels, and keep you from re-hashing reflection calls in your test methods.
Today I put out a new release of the Etsy PHPUnit Extensions. This new release includes PHPUnit_Extensions_Helper_AccessibleObject.
PHPUnit_Extensions_Helper_AccessibleObject is a wrapper for classes which contain private or protected methods that you would like to test in isolation, for some bizarre reason. But, you may only call private and protected methods of the wrapped object that are annotated with @accessibleForTesting.
Why the special @accessibleForTesting annotation on methods in my production code?
With the @accessibleForTesting you will be able to communicate with other developers that while this code is private or protected you intend to test the logic in it. Also, we can easily comb through the code with a static analyzer to find spots in our code that we might want to consider extracting into more reusable places, like another class. Furthermore, we can also sniff for usages of Reflection.* in our tests and useless method overrides, then double back to ensure we move everything to this AccessibleObject method for testing private and protected methods so that we can go back to the loveliness mentioned in the first half of this paragraph.
If you were making the argument for doing the horrible dance to test private methods mentioned in the beginning of this article, then you can certainly get onboard with being more descriptive about your intent with @accessibleForTesting.
For examples, usage, and code: https://github.com/etsy/phpunit-extensions/wiki/Helpers
P.S. This is still naughty! But it’s a compromise I think I might be able to live with.
Alternative Thoughts on Sniffs for Useless Method Overriding
Recently I have been playing with PHP_CodeSniffer.
First I worked on creating a standard for assessing the cleanliness and efficiency of the PHPUnit tests that are supposed to be unit tests. The belief being that if your unit tests pass this standard, then your tests should have taken less time to write, will take less time to debug, and there will be less time spent maintaining because everything in the test would be perfectly straight-forward and pretty much prove that your interfaces for the code under test are simple to use as there would be no funny business need to prep those objects in your test code. This standard will be part of the tool set I will be presenting at PHPNW.
Then I moved on to looking at what sniffs could the whole code base could withstand. Mostly, I focused on sniffs that would save us time during deployment. Sniffs that had interpretation and execution impact with a clear resolution on failure, like no duplicate class names, property names, or parameter names, or non-executable code.
Well, while investigating the latter, I found a new sniff to add to the former: Generic_Sniffs_CodeAnalysis_UselessOverridingMethodSniff
I found several violations for this sniff in our tests, and so I went through and deleted them, but before committing I ran the test suite, and let’s just say it did not go so well. But why?
The tests failed because the overriding was intended to make protected functions in the overridden class public so that the functions were accessible in the scope of the test.
One test file had 46 violations. Each violation translates to 4 lines of code, one for the function signature, one for the call to the parent function, one for the function scope closing brace, and a new-line. This gave a grand total of 184 lines that weren’t really helpful. When I told the developer that wrote this test, he begrudgingly admitted that many times he got stuck doing useless maintenance on the test because he added or changed a new protected method. What a time suck?
If you have read enough testing articles about the virtues of using mock libraries, and know not to write your own mocks, then this may cause you to think that you could just sniff for hand written mocks and not bother with this wonderful sniff, but that’s not the same find.
I will probably write a longer rant on why using anything other than public on a function is over-zealous, pedantic, and a waste of your good time to even think about, but for now let’s take this find as a small bit of proof that it might be better to not think of non-public modifiers on functions, methods, and classes.
So what should the developer have done instead? Have 46 more public functions to muddy the interface for his class?
No. That’s being a little too extreme.
One option is to consider if you were being too DRY. It is quite possible that you did extract method too much and now have 46 one line methods, and 46 one line methods that you do not want to have as part of your public interface is messier than 46 more lines in a single method.
Another option is to consider extracting helper objects. Extracting a bundle of methods, or even a single method, into a new class file acknowledges that that these methods do not define this object, they weren’t part of the public interface anyways, and opens up the potential to re-use those little helpers in other places without forcing re-use through an instance of the original class. Then in tests you can use a lovely mock library to mock these new collaborators and not worry about updating the test unless you are actually adding a new interaction between the object under test and the collaborator.
Once you have your class with only public methods you won’t have to waste your time on useless overriding for the sake of upgrading the method protection level.
I love avoiding lots of time sucking maintenance!
