Wash the method call, which is in the same class that I am testing, is this really a code smell?

Question

I am trying to check the service class (responsible for calling the repository level and performing some operations if necessary), basically this is the class I am trying to check

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 am calling the findById method for the deleteById method, so my questions are.

• Is it really the smell of code to call a method in the same class? I do not think that I should create a separate class to find the car by id.

• How can I make fun of calling the findById method with the deleteById method if I use Mockito.when(carServiceImpl.findById("car1")).thenReturn(carModel); it still calls the method, so I will need to make fun of the call to the repository to search by identifier, even when I already tested the findById method.

Answer 1

This is not necessarily a smell, and you can partially mock the Car as follows:

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

But , if you can let deleteById() execute the “real method”, then your test should have a repository, and in this case, let findById() be the “real call” simply and improve the quality of your test coverage in the absence of any additional costs. The fact that you have already tested findById() does not mean that you should not test it again, indirectly, as part of deleteById() .

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

• Unit test Car , laughing at the repository and using the laughed expectations and checks to test all its methods
• Car Functional / Acceptance Test by providing it with a real repository and using real calls in the underlying repository to validate the actual results for each of its methods.

In a separate note, I suggest that the idea of ​​injecting a repository into a domain object is an intentional use of the “active record” template, in which your entities know how CRUD themselves are. This can be considered a code smell; it violates the PSA and can be considered a poor separation of problems, because the domain subject knows about two things: its own state and how to persist.

Answer 2

You want your test setup and test code to be as "minimalistic" as possible. In this sense, another answer is correct: if you can test deleteById() without special configuration, then go.

And it would findById() that findById() in itself is a "huge" thing that requires a lot of test settings - then I would rather step back and consider the possibility of placing the content of this method in a separate class - a kind of CarIdentityService .

Meaning: very often, when we start making test code more complex, the best answer is to roll back and redesign our production code. In your case, you can push all findById() code into a separate class, and now you can just mock find the search object in the CarService class.

And just for the record: CarIdentityService can be a local or inner CarService class. But introducing this can allow you to optimize your implementation and avoid getting into the spy business.