Mock a method call that is in the same class i'm testing, is it really code smell?

Question

I'm trying to test a service class (responsible for calling the repository layer and do some operations if needed), basically, this is the class I'm trying to test

class CarServiceImpl{
  public Car findById(String id){
      //call repository layer to find a car
  }

  public void deleteById(String id){
      Car car = this.findById(id);
      if(car != null){ 
          //Call repository layer to update the car
      }else{
          Throw NotFOundException();
      }
  }
}

As you can see I call the findById method on the deleteById method, so my questions are.

1. is it really code smell to call a method on the same class? I don think I should create a separate class to find a car by id.

2. how can I mock the call to "findById" on the "deleteById" method, if I use Mockito.when(carServiceImpl.findById("car1")).thenReturn(carModel); it stills call the method so i'll need to mock the call to respository for finding by id too, even when i already tested the method findById.

Answer 1

This isn't necessarily a smell and you can partially mock Car like so:

String carId = "...";
Car car = ...;

CarServiceImpl car = mock(CarServiceImpl.class);
when(car.findById(carId)).thenReturn(car);    
when(car.deleteById(carId)).thenCallRealMethod();

But if you can allow deleteById() to execute the 'real method' then your test must already have a repository in which case letting findById() be a 'real call' is simple and improves the quality of your test coverage at ~no additional cost. The fact that you have already tested findById() doesn't mean that you shouldn't test it again, indirectly, as part of deleteById().

I would suggest that you do either or both of the following:

• Unit test Car by giving it a mocked repository and using mocked expectations and verifications to test all of its methods
• Functional/Acceptance test Car by giving it a real repository and using real invocations on the underlying store to assert actual results for each of its methods

On a separate note, I'm guessing the idea of injecting a repository into a domain object is a deliberate use of the "active record" pattern where your entities know how to CRUD themselves. This could be considered a code smell; it breaches the SRP and could be held to be a poor separation of concerns because the domain object knows about two things: its own state and how to persist itself.

Answer 2

You want your test setup and test code to be as "minimalistic" as possible. In that sense, the other answer is correct in saying: if you can test deleteById() without a special setup then go for that.

And would it turn out that findById() is by itself a "huge" thing that requires a lot of test setup - then I would rather step back and consider putting the content of this method into a distinct class - some kind of CarIdentityService.

Meaning: very often when we start making test code more complex - the better answer is to step back and change the design of our production code. In your case, you might want to push the whole findById() code into a distinct class - and now you can simply mock that finder object within your CarService class.

And just for the record: CarIdentityService could be a local or inner class of CarService. But introducing it might allow you to streamline you implementation and avoid getting into the spy business.