On unit testing private behaviours

Datetime:2016-08-23 02:50:15          Topic: Unit Testing           Share

TL;DR; every time I find myself in a situation where I need to unit test a private behaviour, I consider that a clear indication that I might need to rethink my design.

Let me tell you a story

Some time ago, in what now seems like a galaxy far far away, I was introduced to unit testing by means of someone adding the following requirement to the project my team was developing: “code must be unit tested”.

As you can probably guess by the hint of sarcasm in the previous paragraph, unit testing, like almost everything else in life, is something you learn, not something you are born with. Which was painfully obvious at that time, as I have already written about .

But the thing is, a couple days ago I finally had the opportunity, since I was trapped in a plane for 13 hours, to catch up with some reading. In particular I was finally able to devour Sandy Metz’s new book .

The book is still unfinished, but I highly recommend grabbing a copy, and reading it cover to cover, as soon as possible.

There are plenty of potential quotes in that book, but I would like to focus on one in particular:

… the first step in learning the art of testing is to understand how to write tests that confirm what your code does without any knowledge of how your code does it.

Tests always tell at least two stories.

Unit tests always tell at least two stories. One, specially if the tests are well written, is a cristal clear description of the expectations you have about the behaviour of the code under test.

The second story, though, is usually one that nobody likes to hear. If your tests need to manipulate internal behaviour of the code under test, in order to test your external expectation of that code, then you need to rethink your design. And allow me to illustrate this with an example, that links with the introduction to this very same post you are reading.

When testing goes wrong.

Back to my story: no one in our team had any experience testing, so at the time we did not recognise the code smell, but here is what we were trying to do.

We had this one class that was supposed to be a one to one mapping to a RESTful API, what in the classic three-tier architecture would correspond to the data tier, service layer, transport layer, whatever you call it. The point is, this was the class that performed networking operations against a RESTful API. Let’s call it MediaService.

Part of the expected behaviour of MediaService was that it should cache data, using a key-value cache. This data cache would expire after a given time.

So, the flow would be like this: with every request, we would first check if there was data cached and not expired. If there was data cached, MediaService would return it, if there was no previously cached data or the existing data was marked a expired, MediaService would attack the network, fetch data, and add it to the cache, associating a timestamp to each key-value pair.

So, the next time we wanted to request data to the same endpoint, we would check the cache again, providing a timestamp. The cache would calculate if data was expired or not… rinse and repeat.

In code, that looked similar to the following snippet (those still were the Obj-C times, so this is a rough translation to Swift of my recollection of the events):

/**
 This should be a struct, I know, but those were the dark times, before generics and the coming of value types.
 */
final class CachedItem {
    let value: AnyObject
    let timestamp: NSTimeInterval
    
    init(value: AnyObject, timestamp:NSTimeInterval) {
        self.value = value
        self.timestamp = timestamp
    }
}
 
 
final class TimeCache {
    private let cache = NSCache()
    private let expirationInterval: NSTimeInterval = 1000
    
    func object(key: String) -> AnyObject? {
        let now = NSDate().timeIntervalSince1970
        
        if let cachedItem = cache.objectForKey(key) as? CachedItem where expired(cachedItem.timestamp, now: now) == false {
            return cachedItem.value
        }
        
        return nil
    }
    
    private func expired(timeStamp: NSTimeInterval, now: NSTimeInterval) -> Bool {
        return timeStamp + expirationInterval > now
    }
    
    func set(value: AnyObject, key: String) {
        let cachedItem = CachedItem(value: value, timestamp: NSDate().timeIntervalSince1970)
        cache.setObject(cachedItem, forKey: key)
    }
}
 
// Just to make the code compile
struct Movie {}
struct NetworkClient{}
 
final class MediaService {
    private let cache = TimeCache()
    private let network = NetworkClient()
    
    func getMovie(id: String) -> Movie? {
        let endPoint = "/movie"
        
        if let cachedObject = cache.object(endPoint + id),
            let data = cachedObject as? Movie {
                return data
        } else {
            //Attack the nework, fecth and parse result, and add parsed Movie to the cache
        }
        
        return nil
    }
}

Well, not very idiomatic Swift code, of course, but again, those were the dark days before generics.

Now, if you wanted to test that cache was ignored when cached object have expired… how would you do that?

Dependencies

I don’t recall who said it, but someone said that good software engineering is all about arranging the code in a way that complexity does not collapse on you.

That usually implies breaking down your codebase, organising it into separate subsets or structures. Since you are already breaking down your code and organising it in smaller chunks, you might as well make each of those chunks contain code that is related, that deals with one concern. So, instead of one big ball of code, you would end up having smaller balls, each one of them taking care of one and only one smaller part of the big problem your software is trying to solve. (That’s how I visualise cohesion, by the way)

But of course, the problem your software tries to solve is bigger than each and everyone of those small, cohesive structures that you have used to organise your code. So they need to collaborate with each other.

And here is where dependencies are born. One block of code needs to collaborate with another block of code. One part of your solution needs to collaborate with another part of your solution. Or, if you will, one part of your solution depends on another part of the solution.

The tricky part here is how to set up those dependencies in a way that do not defy the whole purpose of breaking down your code into smaller structures. The key is setting up those collaborations in a way that the collaborators are still independent from each other.

Depending on abstractions not concretions

Yep, that’s kind of the definition of the Dependency Inversion Principle .

In the previous code sample, MediaService needs TimeCache. The key here is that it actually depends on the existence of TimeCache, because MediaService creates TimeCache, therefore the behaviour provided by TimeCache is, in a way, embedded within MediaService. Or, the MediaService abstraction depends on the implementation details of TimeCache.

But, why does that matter?

Firstly, MediaService is tightly coupled to TimeCache. So coupled to it, that it actually needs to create it in order to make itself functional.

Secondly, and due to the previous point, it is not possible to modify the behaviour of TimeCache, without modifying MediaService.

Thirdly, there is a more subtle issue here. The fact that there is a cache, and the fact that said cache expires, is not part of the public API of the MediaService class. It is an implementation detail, buried into the MediaService class code, but an implementation detail that is so significant that leaks outside its container. Because we know MediaService handles a cache that marks items as expired, because we expect actually expect that, however, the expectation is not declared anywhere in MediaService’s public interface.

So now, when it comes to unit testing, we face a very interesting problem: we need to test a behaviour that is a private implementation detail, but since we need to test it, it is not private anymore, it is a well-known expectation, but an expectation that we can not test because it is a private implementation detail. You see the infinite loop here, right?

And notice how I have not even mentioned networking…

No worries, mocks, stubs and spies can help!

Well, not anymore. In the old Objective-C days, we could just declare a category in the testing bundle to expose and override any private property of a class with a mock (or stub, or spy, to be honest, I never really understood the difference).

In this case, we could override cache and network, provide mocks, set our tests, and go on our merry way.

The problem with that is that we would still be testing private behaviour. And why should not do that?

The reason, is again, subtle. When we test private behaviour we are basically coupling our tests to the production code very tightly, because we are not testing public APIs anymore, we are testing internal implementation details we should not even be aware of.

Have you ever heard anyone complaining about how unit testing is a waste of time and effort because if you change the production code there is always going to be a bunch of tests that need to be rewritten? I bet the reason is because of vey tight coupling between tests and the code under testing. Or, in other words, because the tests are not testing why they should (public expectations) but private details about how those expectations are fulfilled.

Also, in the age of Swift, doing that is not even possible anymore. If MediaService is final, we can not subclass and override, and if cache and network are declared as constants with let there is no way to override them.

So, what to do?

Invert the dependencies

This is not the first time I blog aboutdependency injection. Once even using the same example I am using today , another time in adifferent context. And I am afraid it won’t be the last time I blog about it.

But that’s for a reason. Inverting the dependencies, in my experience, only has upsides. It makes code more decoupled, easier to test, and therefore easier to maintain in the long term.

In this case, the best solution would be something along these lines:

protocol Cache {
    func object(key: String) -> AnyObject?
    func set(value: AnyObject, key: String)
}
 
final class TimeCache: Cache {
    /// Implement protocol
}
 
protocol Network {
    func post(args: [String: String])
}
 
final class NetworkClient: Network {
    //Implement protocol
}
 
final class MediaService {
    private let cache: Cache
    private let network: Network
    
    init(network: Network, cache: Cache) {
        self.network = network
        self.cache = cache
    }
}

Cache and networking behaviours can be provide now through the MediaService initialiser. Notice the shift in the way we refer to caching and networking now: as behaviours. Because for MediaService, that’s what they are.

The details on how caching and networking work are not known to MediaService anymore. MediaService is provided the behaviours it needs to rely on, the behaviours it needs to collaborate with.

And that’s the beauty of inverting the dependencies. By carefully setting the way collaborators know about each other, and by carefully hiding whatever is not necessary for collaborators to know, and by shifting from concretions to abstractions we make each of our building blocks rely on the expectations that the other building blocks publish about their own behaviours.





About List