Iceberg Class and Google Unit Testing

Question

I am going through the process of doing unit testing of a existing code which is written not with unit testing in mind.

There is a few class structured like something like this:

class Texture
{
public:
    friend class Model;
private:
 void Load( int a, int b);
 void Update(int a, int b);
 void Use(int a, int b);    
}

class Material
{
public:
    friend class Model;
private:
 void Load(int a);
 void Update(int a);
 void Use(int a);    
}

class Mesh
{
public:
    friend class Model;
private:
 void Load(int a, int b, int c);
 void Update(int a, int b, int c);
 void Use(int a, int b, int c);    
}


class Model
{
    public:

    void Load(); // call all the individual Load()
    void Use(); // call all the individual Use()
}

The reason why they are kept as private is because it is design in such a way where only Model class can call them, hence the friend.

[In the actual code there is a Attorney-Client idiom which limit the access of Model to these class, but I leave it out of the code snippet]

Now I am trying to do unit testing for the classes. While figuring out how to test these private functions, I came across this terminology of an Iceberg Class which I feel the above class is in a way guilty of.

Most article touching on this topic also mentioned that if there is a need to test a private function, it mostly means that class is overdoing and these functions are better off in another standalone class where they stay as public.

So right now, I am not sure if this is a bad code design and I should redesign them to make unit testing easier, or those I just proceed with unit testing as it is.

Would like to hear your opinions

Answer 1

To make this code testable, I'd introduce three pure-virtual interfaces (ITexture, IMesh, IMaterial) and add a free method to create such interfaces (e.g. getTexture) that would return a smart_ptr of type ITexture. Then in a cpp file implement a get[...] method and use it in the production code to create Model object. In unit tests, I'd create a mock for each interface class and set proper expectations on the injected mocks (e.g. using gmock or write your own mock).

Example for Mesh, header file, IMesh.hpp:

class IMesh {
public:
    virtual ~IMesh() = default;
    virtual void Load(int a, int b, int c) = 0;
    virtual void Update(int a, int b, int c) = 0;
    virtual void Use(int a, int b, int c) = 0; 
};
std::unique_ptr<MeshI> getMesh(/*whatever is needed to create mesh*/);

implementaiton file, MeshImpl.cpp:

#include "IMesh.hpp";

class Mesh : public IMesh {
public:
    Mesh(/*some dependency injection here as well if needed*/);
    void Load(int a, int b, int c) override;
    void Update(int a, int b, int c) override;
    void Use(int a, int b, int c) override; 
};
Mesh::Mesh(/*[...]*/) {/*[...]*/}
void Mesh:Load(int a, int b, int c) {/*[...]*/}
void Mesh:Update(int a, int b, int c) {/*[...]*/}
void Mesh:Use(int a, int b, int c) {/*[...]*/}

Dependency injection:

Model model{getMesh(), getTexture(), getMaterial()};

Thanks to this approach one can achieve:

1. Better decoupling - friendship is a very strong coupling mechanism while depending on pure virtual interfaces is a common approach)
2. Better testability - not only for the Model class - because all methods in the interface must be public in order for Model class to use it, you can now test each interface separately
3. Better encapsulation: one can create the required classes only via getter methods - implementation is not accessible to the user, all private stuff is hidden.
4. Better extensibility: now the user can provide different implementations of IMesh and inject it to the model if needed.

For more details on DI techniques, see this question

Answer 2

I'd argue that the use of friend in this case is unfortunate. As I see it, one good use case for friend is, to allow accesses to private elements between classes that conceptually have a tight coupling. When I write they conceptually have a tight coupling I mean that the tight coupling is not a consequence of the use of friend, but the tight coupling between these classes is due to their dependency that is the consequence of their defined roles. In such cases, friend is a mechanism to properly handle this tight coupling. For example, containers and their corresponding iterator classes are conceptually tightly coupled.

In your case it seems to me that the classes are not as tightly coupled on a conceptual level. You are using friend for a different purpose, namely to enforce an architectural rule: Only Model shall use the methods Load, Update and Use. Unfortunately, this pattern has limitations: If you have another class Foo and a second architectural rule that Foo shall only be allowed to call the Use methods, you can not express both architectural rules: If you make Foo also friend of the other classes, then Foo will not only be given access to Use, but also to Load and Update - you can not grant access rights in a granular way.

If my understanding is correct, then I would argue that Load, Update and Use are not conceptually private, that is, they do not represent implementation details of the class that shall be hidden for the outside: They belong to the "official" API of the class, just with the additional rule that only Model shall be using them. Often, private methods are private because the implementor wants to keep the freedom to rename or delete them, because other code just can not access them. This, I assume, is not the intention here.

Taking this all into account, I would argue that it would be better to handle this situation differently. Make the methods Load, Update and Use public, plus, add comments to explain the architectural constraints. And, although my argumentation has not been about testability, this also resolves one of your testing problems, namely allowing your tests also to access Load, Update and Use.

If you also want to be able to mock your classes Texture, Material and Mesh, then take the suggestion from Quarra into account to introduce the respective interfaces.

---

Despite the fact that for your specific example my proposal is to make the methods Load, Update and Use public, I am not an opponent of unit-testing implementation details. Alternative implementations of the same interface have different potential bugs. And, finding bugs is one primary goal of testing (see Myers, Badgett, Sandler: The Art of Software Testing, or, Beizer: Software Testing Techniques, and many others).

As an example, consider the memcpy function: Let's assume you have to implement and test it. You start with a simple solution, copying byte by byte, and you test that thoroughly. Then, you realize that for your 32-bit machine you can do faster if source address and target address are 32-bit aligned: In this case you can copy four bytes at once. When you implement this change, then the new memcpy internally looks quite different: There is an initial check whether the pointer alignment fits. If it doesn't fit, then the original byte-by-byte copy is performed, otherwise the faster copy routine is performed (which also has to handle the case that the number of bytes is not a multiple of four, so there may be some extra bytes to copy at the end).

The interface of the memcpy is still the same. Nevertheless, I think you definitely need to extend your test suite for the new implementation: You should have test cases for two four-byte-aligned pointers, for cases where only one pointer is four-byte-aligned etc.. You need cases where the pointers are both four-byte-aligned and the number of bytes to copy is a multiple of four, and cases where they are not a multiple of four, etc. That is, your test suite will be extended dramatically - only because the implementation details have changed. The new tests are needed to find bugs in the new implementation - although all tests can still use the public API, namely the memcpy function.

Thus, it is wrong to assume unit-testing is not about implementations details, and also it is wrong to assume that tests are not implementation specific only because they test via the public API.

It is correct, however, that a test should not unnecessarily depend on implementation details. Always try first to create useful tests that are implementation agnostic, and later add the tests that are implementation specific. For the latter, testing private methods (for example from a friend test class) can also be a valid option - as long as you are aware of the disadvantages (test code maintenance will be needed if private methods are renamed, deleted etc.) and weigh them against the advantages.