I’m a big fan of unit tests, and tests in general. I wrote an article about unit tests previously, and although it’s not very complete, I wanted to write a follow up talking about unit testing anti-patterns.
Testing private/protected methods directly
I think one of the most asked questions when it comes to unit tests is: “Should I test private/protected methods directly?” and the answer is generally “No”…but it’s not that simple.
Lets firstly look at why you shouldn’t and then look at when it’s okay to break that rule.
Normally private(I’ll include protected methods when saying private just so I don’t have to write private/protected all the time, so keep that in mind) methods shouldn’t be tested directly. They should, however, be tested indirectly by other public methods that call said private method.
If you find it hard to properly test a private method indirectly, then it may mean that either that private method is dead code, and should in fact be removed, or you have a missing abstraction.
Generally a well designed API(when I say API I don’t mean only the web API you may be thinking of, public methods in a class represent the API of that class, it’s the way that class communicates with the outside world) will hide implementation details and provide some public way to input data and get back processed data(or data in general I suppose). Think, for example, of the way you’d turn on your car: you put the key in(or press a button depending on the model) and your car starts. You, the driver, care very little of the bits and bobs that make the car start, you only care that it starts correctly. That’s what a well designed API is, it hides implementation details while offering a some public method(the button in our example) of doing whatever it is supposed to do.
In unit testing(I’m specifically talking about Detroit or classical style of unit testing here) you’d test a specific behavior by replacing outside dependencies(like a database) via mocks and leaving any class that’s not outside the scope of the system as is. Testing a private method would involve calling it indirectly via some public function, as previously mentioned. Lets see an example of that.
The best example I can think of is in fact one that I created myself, unwillingly. It involved some complex price calculations on some data received from an API. For brevity sake I won’t include the whole class here because it’s quite long, I’ll just include the important bits.
public String computeSellableItem(String price) throws InvalidCurrencyException, InvalidPriceException {
...
// some transformation that transform the String price into a BigDecimal
resultingPrice = calculatePrice(price, currencyRatio);
sellableItemDTO.setSoldQuantity(1);
sellableItemDTO.setPrice(resultingPrice);
return sellableItemDTO;
}
private BigDecimal calculatePrice(BigDecimal price, BigDecimal currencyRatio) {
BigDecimal discounts = calculateDiscount(price, currencyRatio);
BigDecimal taxes = calculateTaxes(price, currencyRatio);
return /* do some math of the BigDecimals */
}
private BigDecimal calculateDiscount(BigDecimal price, BigDecimal currencyRatio) {
double discount = /* some complex logic to calculate discount */
return new BigDecimal(discount);
}
private BigDecimal calculateTaxes(BigDecimal price, BigDecimal currencyRatio) {
double taxes = /* some complex logic to calculate taxes */
return new BigDecimal(taxes);
}
Leaving aside the questionable quality of the code you’ll immediately notice that calculatePrice does some complex logic that involves 2 private methods(3 if you include calculatePrice itself). This method contains critical business logic that the system cannot work properly without, therefore it’s very import to be tested thoroughly.
Now, this implementation isn’t bad in and of itself but it does make testing it hard. This doesn’t mean that you should write your code to make it easier to test but it may point to a different issue. This code can be extracted to its own class, given how important calculating the price is to the application itself it’s a justifiable(if not necessary) extraction. This is the missing abstraction I was talking about earlier.
public class PriceCalculator {
public BigDecimal calculatePrice(BigDecimal price, BigDecimal currencyRatio) {
BigDecimal discounts = calculateDiscount(price, currencyRatio);
BigDecimal taxes = calculateTaxes(price, currencyRatio);
return /* do some math of the BigDecimals */
}
private BigDecimal calculateDiscount(BigDecimal price, BigDecimal currencyRatio) {
double discount = /* some complex logic to calculate discount */
return new BigDecimal(discount);
}
private BigDecimal calculateTaxes(BigDecimal price, BigDecimal currencyRatio) {
double taxes = /* some complex logic to calculate taxes */
return new BigDecimal(taxes);
}
}
Now the price can be calculated independently, and we have the added benefit of using output based testing. Furthermore because PriceCalculator doesn’t have any hidden outputs or inputs, we know exactly what we put in and we get out.
Thus, if you take anything away so far it’s this: if you find yourself wanting to test private methods think if you may have a missing abstraction OR if that’s dead code, in which case it should be deleted.
However, there are cases when testing private methods is acceptable. Even more so, testing private methods in and of itself isn’t wrong per-se. The issue is that private methods are usually the path to implementation details, details which you should NOT care about. Testing implementation details leads to test brittleness. As soon as the details are refactored, your tests will fail, even though the end result has not changed. There’s a reason well designed APIs hide implementation details(this is what encapsulation is when writing good classes).
Ok, so when is it okay to test private methods then?
When testing private methods doesn’t lead to test brittleness, and when that private method is part of an observable behavior.
This is a real example I encountered at my job. We have a client application that calls our backend for some data. The client app doesn’t know how our entities are structured(nor should it), but it does need to know if an input field should be multi-select(checkbox) or a single select(radio button) or some other input type. To communicate this to the client we use some metadata that’s returned along with the actual data. Each endpoint has it’s own metadata that tells the client how to structure various input. We also have custom rules, for example if an input field is multi-select then another specific field in the metadata is required.
Each entity representation has its own ruleset that is represented by fields in that entity, but at the same time each representation is represented by common client fields(example checkbox, radio button, etc).
Lets see an example.
abstract class Displayable
{
abstract protected function getSettings();
private function validate(string $key, string $inputField) : void
{
if(empty($key) && $inputField == DisplayVariants::VARIANT_AUTOCOMPLETE) {
throw new ACustomException("...");
}
}
protected function field(string $key, string $inputField/*...more params here*/) : array
{
$this->validate($key, $inputField);
//...more code here
}
public function getMeta() : array
{
return $this->getSettings();
}
}
class DisplayableEntity extends Displayable
{
protected function getSettings() : array
{
return [
$this->field("EntityId", DisplayVariants::VARIANT_CHECKBOX),
// ...more harcoded fields
];
}
}
In this code example we have DisplayableEntity that extends Displayable. Each entity that extends Displayable defines its own fields…but the validation is done in Displayable because the rules are common for all entities. Taking advantage of polymorphism, the method getMeta is part of Displayable which will forward the call to one its children.
But you may notice that if everything is written correctly then the method validate will never fail and thus it will never throw an exception. However, it MAY throw an exception at some point if children of Displayable define incorrect fields.
Well, this is an issue isn’t it? Since each child defines hardcoded fields and they’re correct, the method validate can NEVER have all of it’s branches tested using code as it exists in production, because the error would be caught before it reaches production.
This is a good example of code that can have its private methods tested. Making validate public via the magic of reflection in tests, won’t lead to test brittleness since we’re not making the test reliant on implementation details, it has good business value since we want to test all the branches in that method and that method is also part of a very observable behavior, that is it will throw an exception when some field is given incorrect parameters.
Now, you may argue that the code is written poorly or can be rewritten, and I won’t contest you on that, but keep in mind that a rewrite may not be feasible. So you have to work with what you have.
So to summaries this particular issue: do test private methods if it doesn’t lead to test brittleness, and when that private method is part of an observable behavior.
Breaking encapsulation for the sake of easy tests.
Another common anti-pattern is writing code for easy testing rather than writing code for business requirements.
I’ll use the previous code example with Displayable. You may think to yourself why not simply change the validate method to public visibility? And the answer is that(and I cannot stress this enough): your tests must interact with whatever it is you are testing exactly as if you are testing code in production. You’re not writing test for lower environments after all. You want to test the code that ends up in production. And it makes zero sense to make that specific method(validate) public.
At the end of the day even though we like writing code, the code is not the thing that makes the big bucks. It’s just a means to make those big bucks. And having good quality code means you won’t have to do maintenance on it very often. And encapsulation is a big part of that quality.
Exposing implementation details to tests
Lets assume we have a very simple calculator implementation(that for some reason can only add numbers)
class Calculator
{
public function add(int $a, int $b): int
{
return $a + $b;
}
}
And lets write a test for it
class CalculatorTest
{
public function adding_two_numbers()
{
$a = 1;
$b = 2;
$expected = $a + $b; <-- implementation leak
$actual = (new Calculator())->add($a, $b);
assertEquals($expected, $actual);
}
}
The test itself is perfectly valid. It uses real objects, it tests the full path. We could argue that we can use parameterized values so we cover more scenarios, and that’s a valid concern, but for our purposes the test is fairly complete.
However there’s a small issue. Notice that the $expected is in fact a 1:1 copy of our algorithm used to calculate the end result.
Obviously in this particular example it’s just one line of code so it’s not very impactful, but imagine a more complex method. Copying the whole algorithm leaks implementation details to the test(not to mention it pollutes the tests with code).
Tests should have zero idea of implementation(unless you’re testing specific steps, in which case you’d use mocks), you don’t need to have the algorithm in the test. All you need is the expected result in its final form.
Lets rewrite the test to mirror that.
class CalculatorTest
{
public function adding_two_numbers()
{
$a = 1;
$b = 2;
$expected = 3; <-- implementation no longer leaked
$actual = (new Calculator())->add($a, $b);
assertEquals($expected, $actual);
}
}
Now there are no leaks of implementation to the test, we also have the added benefit of having less code to write.
We can do even better, lets use the idea of parameterized tests to cover more scenarios.
class CalculatorTest
{
// this won't actually work since the syntax is wrong
// it's just to illustrate the idea
#Parameterized(1,2,3)
#Parameterized(100,200,300)
#Parameterized(-10,-100,-110)
#Parameterized(-10, 10,0)
public function test_adding_two_numbers($a, $b, $expected)
{
$actual = (new Calculator())->add($a, $b);
assertEquals($expected, $actual);
}
}
Code pollution
This is a surprisingly used antipattern. Adding code that is not actually used in production but is there just to help with testing.
If you find yourself using flags in classes just to help with testing then something’s wrong.
If you can extract whatever it is you need to an interface and mock that. Arguably this is another form of code pollution since the interface may only be used for mocks in tests, but I find it to be less damaging that having switches to check if the environment is production or non-production based on some constant or whatever in some .env file or some .properties file.
I’m sure there are more anti-patterns out there which I have yet to encounter. These are simply the ones that I’ve encountered the most often.
As with all things in programming, at the end of the day practical solutions will always trump academic thoughts, so if you feel like you need to test that private method then do that. Breaking encapsulation can be valid in some scenarios. Just make sure you have good reasons to do either of those things.