I am working on a project in which we have to implement and unit test some new module. I had a quite clear architecture in mind so I quickly wrote down the main classes and methods and then we started writing unit tests.
While writing the tests we had to make quite a few modifications to the original code, such as
- Making private methods public in order to test them
- Adding extra methods to access private variables
- Adding extra methods to inject mock objects that should be used when the code runs inside a unit test.
Somehow I have the feeling that these are symptoms that we are doing something wrong, e.g.
- the initial design was wrong (some functionalities should have been public from the beginning),
- the code was not designed properly for being interfaced with unit tests (maybe due to the fact that we started to design the unit tests when quite a few classes had been designed already),
- we are implementing unit tests the wrong way (e.g. unit tests should only directly test / address the public methods of an API, not the private ones),
- a mixture of the above three points, and maybe some additional issues I have not thought about.
Since I have some experience with unit testing but I am far from being a guru, I would be very interested to read your thoughts regarding these issues.
Beside the above general questions, I have some more specific, technical questions:
Question 1. Does it make sense to directly test a private method m of a class A and even make it public in order to test it? Or should I assume that m is indirectly tested by unit tests covering other, public methods that call m?
Question 2. If an instance of class A contains an instance of class B (composite aggregation), does it make sense to mock B in order to test A? My first idea was that I should not mock B because the B instance is part of the A instance, but then I started to doubt about this. My argument against mocking B is the same as for 1: B is private wrt A and only used for its implementation, therefore mocking B seems like I am exposing private details of A like in (1). But maybe these problems indicate a design flaw: maybe we should not use composite aggregation but a plain association from A to B.
Question 3. In the above example, if we do decide to mock B, how do we inject the B instance into A? Here are some ideas we had:
- Inject the B instance as an argument to the A constructor instead of creating the B instance in the A constructor.
- Pass a BFactory interface as an argument to the A constructor and let A use the factory to create its private B instance.
- Use a BFactory singleton that is private to A. Use a static method A::setBFactory() to set the singleton. When A wants to create the B instance it uses the factory singleton if it is set (the test scenario), it creates B directly if the singleton is not set (the production code scenario).
The first two alternatives seem cleaner to me, but they require changing the signature of the A constructor: changing an API just to make it more testable seems awkward to me, is this a common practice?
The third one has the advantage that it does not require changing the signature of the constructor (the change to the API is less invasive), but it requires calling the static method setBFactory() before starting the test, which is IMO error-prone (implicit dependency on a method call for the tests to work properly). So I do not know which one we should choose.