Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

  1. the initial design was wrong (some functionalities should have been public from the beginning),
  2. 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),
  3. 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),
  4. 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.

share|improve this question
    
I think C++'s friend class/function feature could be of use. Have you tried that? –  Mert May 28 '12 at 7:53
    
@Mert: We haven't tried it. Question: By using friend we would have to declare test code classes as friend of main code classes. Is this OK? We would have production code depending on test code. Is this a good idea? Or was it another solution you had in mind? –  Giorgio May 28 '12 at 8:05
    
I am not a C++ expert, it just came to my mind as a simple solution. –  Mert May 28 '12 at 8:24
add comment

3 Answers

You should look up Dependency Injection and Inversion of Control. Misko Hevery explains a lot of it on his blog. DI and IoC is effectively a design pattern for making code that is easy to unit test and mock.

Question 1: No, do not make private methods public to test them. If the method is sufficiently complex, you can create a collaborator class that contains just that method, and inject it (pass into the constructor) into your other class. 1

Question 2: Who constructs A in this case? If you have a factory/builder class that constructs A, then there is no harm in passing the collaborator B into the constructor. If A is in a separate package/namespace from the code that uses it, you can even make the constructor package-private and make it so the factory/builder is the only class that can construct it.

Question 3: I answered this in Question 2. Some extra notes though:

  • Using a builder/factory pattern allows you to do dependency injection as much as you want, without having to worry about making code using your class difficult to use.
  • It separates object construction time from object use time which means that code using your API can be simpler.

1 This is the C#/Java answer - C++ may have extra features that facilitate this

As an answer to your comments:

What I meant was that your production code would be changed from (please forgive my pseudo-C++):

void MyClass::MyUseOfA()
{
  A* a = new A();
  a->SomeMethod();
}

A::A()
{
  m_b = new B();
}

to:

void MyClass::MyUseOfA()
{
  A* a = AFactory.NewA();
  a->SomeMethod();
}

A* AFactory::NewA()
{
  // Construct dependencies
  B* b = new B();
  return new A(b);
}

A::A(B* b)
{
  m_b = b;
}

Then your test can be:

void MyTest::TestA()
{
  MockB* b = new MockB();
  b->SetSomethingInteresting(somethingInteresting);

  A* a = new A(b);

  a->DoSomethingInteresting();

  b->DidSomethingInterestingHappen();
}

In this way, you don't need to pass the factory around, the code calling A doesn't need to know how to construct A, and the test can custom-construct A to allow behaviour to be tested.

In your other comment, you asked about nested dependencies. So, for example, if your dependencies were:

A -> C -> D -> B

The first question to ask is if A uses C and D. If they aren't, why are they included in A? Assuming they are used, then perhaps it is necessary to pass in C in your factory, and have your test construct a MockC that returns a MockB, allowing you to test all possible interactions.

If this is starting to get complicated, it might be a sign that your design is perhaps coupled too tightly. If you can loosen the coupling and keep cohesion high then this kind of DI becomes easier to implement.

share|improve this answer
    
Regarding answer 2: Do you mean that the production code and the test code would use two different factory implementations to construct A, where (1) the production code factory would inject the production B instance and (2) the test code factory would inject the B mock instance. Actually, the B instance is quite deeply nested in a composition tree. I would have to pass the factory down through several levels of the composition tree. Instances of A are constructed by their parent object (some other class) –  Giorgio May 28 '12 at 10:36
    
Regarding question 3, one of our issues is how the B factory should be injected into A: using a constructor argument in the A constructor to sets a local reference to the factory, or as a singleton that A accesses when it needs to use the factory. –  Giorgio May 28 '12 at 10:44
    
@Giorgio Have a look at my update wrt your comments. Not knowing your specific example, my generic examples may not apply, but this is the kind of approach I would take to seeing if I can simplify the testing problem. –  Bringer128 May 29 '12 at 3:28
    
Thanks a lot for your example (I find the pseudocode OK). Two observations: (1) Why use a factory in the production code and a plain constructor in the test code? (2) The composition hierarchy is C -> D -> A -> B, and the user of C must provide the MockB instance that must be injected from C into A. –  Giorgio May 29 '12 at 5:05
    
(1) The factory is to hide the DI aspect from the code that uses A. It is intended to prevent using code becoming any more complicated for adding DI. To be more precise, it allows abstracting the dependency management away from A, B, even C and D. (2) The example you are giving is not really a unit test per-se. If you're invoking methods on C only it will be much harder to get high test coverage on A. Up to you how important this is, but a unit test should only test A, its interactions with B and its return values. –  Bringer128 May 29 '12 at 5:53
add comment

To Question 1: That depends. Typically, you start with unit tests for public methods. Sometimes you encounter a method m you want to keep private to A, but you think that it makes sense to test m in isolation either. If that's the case, you should either make m public or make the test class TestA a friend class of A. But beware, adding a unit test for testing m makes it a little bit harder to change the signature or the behaviour of m afterwards; if you want to keep that an "implementation detail" of A, it may be better not to add a direct unit test.

Question 2: (C++ build-in) composite aggregation does not work well when it comes to mock out an instance. In fact, since the construction of the B happens implicitly in the constructor of A, you have no chance to inject the dependency from outside. If that is a problem depends on the way you want to test A: if you think it makes more sense to test A in isolation, with a mock of B instead of B, better use a plain association. If you think you can write all the needed unit tests for A without mocking out B, then a composite will probably be ok.

Question 3: changing an API to make things more testable is common as long as you don't have much code so far relying on that API. When you are doing TDD, you don't have to change you API afterwards to make things more testable, you start initially with an API designed for testability. If you want to change an API later to make things more testable, you may encounter problems, that's true. So I would go with the first or second of the alternatives you described as long as you can change your API painlessly, and use something like your third alternative (remark: this works also without the singleton pattern) only as a last resort if I must not change the API under any circumstances.

About your concerns that you might "do it wrong": every big engine or machine has maintenance openings, so IMHO the need of adding something like that to software is not too surprising.

share|improve this answer
2  
+1 For the last paragraph. For a good exapmple study how the electronics world tests? –  mattnz May 28 '12 at 9:12
    
+1: Thanks for a very well-motivated answer. One of my main concerns is that application code should provide functionality to application code, not to test code: the test code should observe the application code without imposing requirements in it. Of course, you might have some requirements to make the code more observable, but these should be really minimal. See the composite example: IMO one should choose composite wrt plain association based on application domain requirements, not on testability. IMO bending application requirements to test requirements should be the last resort. –  Giorgio May 28 '12 at 9:41
1  
@Giorgio: your misconception here is that using an association vs a composite has anything to do with domain requirements - you can fulfill any domain requirements with both kind of design. Making software more testable is not something you can expect to be achieved just by making minimal changes. If you do it right, it will definitely influence your software at the design level. –  Doc Brown May 28 '12 at 10:21
    
@Doc Brown: Well, if A <>- B is a composite, instances of B can only exist if managed by instances of A and their lifetime is controlled by the lifetime of A. This can be a domain requirement. On the other hand, a plain "use" association A -> B does not impose that an A instance should manage a B instance. Maybe in our case there really was a flaw in the analysis of the application domain (we should use an association instead of composition). –  Giorgio May 28 '12 at 10:33
    
@Giorgio: you may have a requirement for instances of A and B to have the same lifetime. But how you fulfill it is yours, there is no domain requirement that forces you to solve this by using the C++ build-in form of composition. If you want to be able to test A and B in isolation, then - at least for your test - you need to have an instance of A without B and vice versa. So for this case, you better use a run-time mechanism to control the lifetime of those objects (for example, using smart pointers) instead of a compile time mechanism. –  Doc Brown May 28 '12 at 10:49
show 2 more comments

I think testing public methods are enough most of the time.

If you have a great deal of complexity in your private methods, then consider putting them in another class as public methods and use them as private calls to those methods in your original class. This way you can ensure both methods in your original and utility classes work correctly.

Relying heavily on private methods is something to consider about design desicions.

share|improve this answer
    
I agree with you: if one feels the need to test a private method, then maybe it should not have been private in the first place and one should put it in a separate utility class that should be tested separately. –  Giorgio May 28 '12 at 8:28
    
Testing all public methods should already test (read: cover) all private methods. Else, what are they doing there? :) –  Amadeus Hein May 28 '12 at 8:28
    
@Amadeus Hein: I agree again. But one could argue that it is more robust to test private methods separately. –  Giorgio May 28 '12 at 8:30
2  
@Mert Yes, but in general, wanting to test a private method is a signal something else is wrong in the code. More detail: link –  Amadeus Hein May 28 '12 at 8:38
1  
"Relying heavily on private methods is something to consider about design desicions.": In our case the private methods were utility methods that are called once by a public method. But then we wondered whether it would be more robust to test them as well. But probably, as many pointed out, it would make sense to move these methods into a utility class and make them public if they are so critical that one wants to test them. –  Giorgio May 28 '12 at 9:47
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.