1
\$\begingroup\$

I'll write a small simulation of my actual class first to request insights on unit testing one of the behaviors of this class. Here is my class A:

public class A {
    private final String a;
    private final String b;
    private String c;

    public A(final String a, final String b) {
       this.a = a;
       this.b = b;
       this.c = null;
    }

    public getC() {
       if (this.c==null) {   
          // Logic to compute value of this.c using this.a and this.b
          // set this.c
       }

       return this.c; 
    }
}

In the above class I'm doing lazy initialization of a data member c. Responsibility of class A is to compute this.c from this.a and this.b and cache it so that there is no need to compute it again on future invocation of getC. Class A has only this responsibility.

I want to write a unit test to verify that on Multiple invocations of getC() method on the same instance of class A, logic for computing this.c should be executed only once. Could anyone please help in figuring out, how can I write this unit test ? One way I can see is delegating the logic of computing this.c to another class B and then making class A dependent on class B so that a mock of class B can be injected during testing. After injection of mock of B, Mockito's feature to verify number of times method invocation happens on a mock can be used. But Class A has only one responsibility of computing this.c and delegating that responsibility to another object is looking like an overkill to me.

\$\endgroup\$
6
  • \$\begingroup\$ Is this logic in a method of the class or not? \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 17:27
  • \$\begingroup\$ Lets put the logic in a private method if it makes it easier to test how many times the method will be invoked on multiple invocations of getC on same instance of class A \$\endgroup\$ Commented Jun 7, 2013 at 17:28
  • \$\begingroup\$ The problem here is with private. While you can spy them, it is extremely painful to do... But hold on, I am writing an answer \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 17:29
  • \$\begingroup\$ Btw I'm not aiming at testing the logic to compute this.c, that can be tested by testing getC itself. I'm interested in making sure that logic is executed only once on multiple invocations of getC on same instance of class A \$\endgroup\$ Commented Jun 7, 2013 at 17:31
  • \$\begingroup\$ Yes, and private is a problem here \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 17:32

2 Answers 2

2
\$\begingroup\$

Delegate the computation of a and b to a method, say, .compute(). Since we want to spy this method's execution, we will suppose that we have Guava and its useful @VisibleForTesting annotation, and make the method protected:

@VisibleForTesting
protected String compute(String a, String b) { // code here }

Now, code written using Mockito and TestNG (with test file in same package than class A, so that the compute() method is visible):

@Test
public void computationOfCHappensOnlyOnce()
{
    final String a = "whatever";
    final String b = "you want";

    final A spy = spy(new A(a, b));

    for (int i = 0; i < 10; i++)
        spy.getC();

    verify(spy, times(1)).compute(eq(a), eq(b));
}

Note: the way your class is currently written, it is NOT thread safe. The quick, coarse way to fix it is to make .getC() synchronized.

EDIT Discussion on a different design to get the value of c

This requires a creation of a new factory class for instances of class A; it would have the responsibility to create new instances and compute c:

// if the class is a singleton...
final FactoryForA factory = FactoryForA.getInstance();

final A a = factory.newA(a, b);

// This calls back to the factory to compute c:
a.getC();

The A class needs to implement .equals() and .hashCode() (on fields a and b but obviously not c!). Also, it has a constructure which is now package private (so that the factory can access it) and whose prototype is:

final String a, b;
final FactoryForA factory;
// No `c` member, see below

A(String a, String b, FactoryForA factory) 
{
    this.a = a; this.b = b; this.factory = factory;
}

public getC()
{
    return factory.computeC(this);
}

Now for the factory itself: it has an ExecutorService and a Map<A, FutureTask<String>>. Here is the code of computeC():

// instance variables of the factory
private final ExecutorService service
    = Executors.newFixedThreadPool(5); // for example
private final Map<A, FutureTask<String>> map = new HashMap<>();

// package private
String computeC(final A a)
{
    FutureTask<String> task;
    // Synchronized map access
    synchronized(map) {
        // If there is no task created for that instance of A, create it
        // and submit it for execution
        task = map.get(a);
        if (task == null) {
            task = createFutureTask(a); // TODO, but nothing too complicated
            // IMPORTANT! If you don't do this, the task is not executed
            // and .get() below will block indefinitely
            service.execute(task);
            map.put(a, task);
        }
    }
    // A FutureTask will keep its status! We can therefore call .get()
    // as much as we like; the computation will only have happened once
    return task.get();
}

One advantage is that if two instances created for A are equal (ie, the same a and b parameters), computation for c happens only once!

\$\endgroup\$
11
  • \$\begingroup\$ Thanks for your answer. I guess making compute method package-private instead of protected should also work.Also it looks like there is no need for compute method to take a and b as input because they are already member of class A. On a larger picture do you see a need for redesigning this class. Mockito documentation is hinting towards the fact that if you are spying in unit tests, there is some code smell around. \$\endgroup\$ Commented Jun 7, 2013 at 18:02
  • \$\begingroup\$ Well, you need to verify that .compute() is called with the correct arguments ;) Out of the class you cannot know that. As to redesign, yes I do have an idea... I'll edit the post and tell you when done. Will take a little time though ;) \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 18:06
  • \$\begingroup\$ OK, done. Note that if you use Guava, you can advantageously this crude cache of mine by a LoadingCache. \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 18:31
  • \$\begingroup\$ I guess FactoryForA which you mentioned is same as class B which I mentioned in my question.Delegating the logic of computation of c to other class B aka factory is looking like an overkill to me because class A has a single responsibility i.e. to generate c.If we remove this logic of computation from class A, there will be nothing left in class A apart from one line of code of delegation of responsibility to class B aka FactoryA. \$\endgroup\$ Commented Jun 7, 2013 at 18:47
  • \$\begingroup\$ Ah, I didn't know that this was class A's only purpose... But you want to keep the results of c cached, is that right? \$\endgroup\$
    – fge
    Commented Jun 7, 2013 at 18:53
1
\$\begingroup\$

Why not keep this simple and test wether subsequent invocations really do return the same instance as before :

A a = new A("a", "b");
assertTrue(a.compute() == a.compute());

This way, there is no need to delegate to another class, nor the need to make implementation details public or protected.

Remember that you should test behaviour, and not implementation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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