Partial Mocks are bad, why exactly? [closed]

问题:

Scenario

I have a class call it Model that represents a complex, composite object of many other objects of varying types. You can think of it as a Car which has Door[], Tire[], Engine, Driver etc. And these objects in turn have sub objects such as Engine has SparkPlug, Clutch, Generator etc.

I have a Metrics class which calculates some more or less complex metrics about the Model, in essence it looks something like this:

public class Metrics{
    private final Model model;

    public Metrics(Model aModel){model = aModel;}

    public double calculateSimpleMetric1(){...}
    public double calculateSimpleMetric2(){...}
    public double calculateSimpleMetricN(){...}

    public double calculateComplexMetric(){
        /* Function that uses calls to multiple calculateSimpleMetricX to 
           calculate a more complex metric. */
    }
}

I have already written tests for the calculateSimpleMetricX functions and each of them require non-trivial, but manageable amounts (10-20 lines) of setup code to properly mock the related parts of the Model.

Problem

Due to the unavoidable complexity of the Model class stubbing/mocking all the dependencies for calculateComplexMetric() would create a very large and difficult to maintain test (over 100 lines of setup code to test a reasonable representative scenario, and I need to test quite a few scenarios).

My thought is to create a partial mock of Model and stub the relevant calculateSimpleMetricX() functions to reduce the setup code to a manageable 10 or so lines of code. As these functions are already tested separately.

However, Mockito documentation states that partial mocks are a code smell.

Question

Personally I would cut the corner here and just partial mock the Metrics class. But I'm interested in knowing what is the "purist" way to structure this code and unit test?

Solution

I ended up doing splitting into smaller, more cohesive classes as suggested by the accepted answer and using dependency injection:

public class AbstractMetric{
    abstract public double calculate();
}

public class ComplexMetric extends AbstractMetric{
    private final SimpleMetric1 sm1;
    private final SimpleMetric2 sm2;

    public ComplexMetric(SimpleMetric1 asm1, SimpleMetric2 asm2){
        sm1 = asm1;
        sm2 = asm2;
    }

    @Ovrerride
    public double calculate(){
        return functionof(sm1.calculate(), sm2.calculate());
    }
}

It is testable, and I can let each implementation of AbstractMetric implement caching if they need to to improve performance at a later stage. I find that adding the metrics to the constructor is acceptable comparing to passing them along to the calculate function in every call. Even this can be hidden with a factory method.

回答1:

I'm guessing that the reasoning behind this can be as follows: if you need to partially mock a class to test something ignoring a part of its behavior then this class is doing more than one thing. This violates the Single Responsibility Principle, and that is a code smell.

Also if an object can do anything useful with its internals partially stubbed (thus, being effectively non-existent) then this object has low cohesion, which also is a code smell.

Your Metric class is doing more than one thing - it calculates different types of metric. Consider refactoring it into an abstract Metric and appropriate subclasses - each calculating its own metric.

Some arguments for partial mocks usefulness can be found here. However they are all founded on a scenario when SRP is already violated and you can't do anything about it.

回答2:

Due to the unavoidable complexity of the Model class stubbing/mocking all the dependencies for calculateComplexMetric() would create a very large and difficult to maintain test (over 100 lines of setup code to test a reasonable representative scenario, and I need to test quite a few scenarios).

Who says you need to mock/stub all the dependencies for calculateComplexMetric() anyway? You're asking about the purist way to test this. If you don't mean the mockist way, then it would be by mocking the "ports" (UI, DB etc.) to your core application code and unit testing several classes together. This could solve your problem if your application already has code to construct a real Model based on a smaller amount of setup. I'd prefer to go that way if possible.

Otherwise, and if you want to keep those methods together in this class (which could make perfect sense), you can do something which is probably against everyone's advice (and is certainly not purist), but could as well be perfectly fine if calculateComplexMetric() can be implemented in a completely stateless way given the results of the simple metrics: create a static method (could be public or even just package visible and does not need to be in the same class) that implements the calculation with the simple metrics as parameters. That should make it real easy to test. The original calculateComplexMetric() just calls the simple methods and delegates to the static method. Its implementation can be so simple it doesn't need to be explicitly tested in isolation.

The "disadvantage" of that is that the static method is more difficult to mock. But that shouldn't be necessary since it doesn't cross any port (e.g. access DB or call a web service). But anyway, this is probably as far from purist as you can get.

Lastly, instead of making this a static method, you can consider adding it just as a regular method inside your Metrics class. You can even make it package private if you don't want to expose it and your test is in the same package.

回答3:

How many calculateSimpleMetricX methods are we talking about? If it is only a few, perhaps it's worth considering just passing the returned values of the simple metrics as parameters tocalculateComplexMetric

public double calculateComplexMetric(double simple1, double simple2, ...)

If there are too many parameters, it might be worth encapsulating the values of the simple metrics into a new class SimpleMetrics and pass that in instead:

public double calculateComplexMetric(SimpleMetrics metrics){
    double sm1 = metrics.get1();
    double sm2 = metrics.get2();
    ...
}

Either way, it is now easy to test the complex metrics computation because you are providing the values for the simple metrics already without mocking.