Since there are already great articles on this topic(please see resources in the end) in this post a more practical way(source) is shown how I was screwing up tests. So we are going to develop a small JAVA application and show step by step what can go wrong even without noticing it.
Motivation
Now days testing is inseparable part of every developer job. Is not only the companies understand the importance of testing but also we developers finally understand that testing our code in the end makes our life easy. At least is supposed to in theory or in the great books. Practically it happens rarely that tests are implemented the way they are supposed to. This causes delays,high developing cost and deaths of many projects instead of the opposite. So how we are testing our application has great impact.
Who is the product?
We all agree that the product is the running software on production which provides certain services or value for his users or customers.Well there is another way we can see that from the test perspective.
Lets suppose we have very good tests for every use case and this tests define precisely how the product behaves and in same time how the product is not suppose to behave.So basically we have a list rules which draw how our software should act precisely and flag as soon as possible when on of the constraints is violated.
Now lets suppose we hire a brand new team and give them only the tests and ask them to write a software which passes all those tests. The team can choose every technology it likes as long as the tests are green.
Finally the team passes all the tests and we deliver this brand new software in production. Did the customer notice any difference? Not even one! From their perspective this software is still doing the same things and is offering same services. So one can argue who really is the product, the software which is replaceable or actually the tests or set of rules describing how the software behaves.
So basically as long as tests are describing what our software is doing we can be independent from the implementation which brings us to next topic , the root of the problem
Test What not How
This sounds easy enough but actually is the root of the problem with the most of the tests we write. Why? Because most of the time even without noticing it we are testing Implementation Details rather than the Behavior of our software/module/class. This lead to the state where non functional(behavior) changes can cause our tests to break.Even simple refactoring like changing types or moving code around will make a lot of tests to fail and a lot of waste time to fix them again and again, even if no requirements or behavior was changed. The reason is because our tests are coupled with implementation details which is normal to change in time as the software improves further.We should test our code like a black box in a sense that we do not care what is inside but rather interested to fully describe the behavior(what will the outputs for certain inputs).
Most of the time I found that I was actually testing HOW the code was doing certain things rather than WHAT is doing. There may be a lot of reason behind that but from my personal experience I was doing this because I was misunderstanding TDD and also follow blindly follow the rule : Test Every CLASS and Every Method inside.
Although we may agree with this rule for a moment there are still some questions :
How is one going to detect Implementation Details Vs Behavior?
How am I going to detect my self violating the holy rule?
Example
Although current example code is not real is inspired from many such. Lets suppose we have an existing very simple back-end application which registers persons into Database. For simplicity lets for a moment remove the Controller layer or REST part of our application. So we have only : Service , Data Access Objects(DAOs), JPA+DB.
If we look at our existing PersonService class is fairly simple. It has to public methods : savePerson and updatePerson.
- SavePerson method before saving person it validates if all persons required fields are correctly filled, than converts Person object to a database object PersonDbo and finally calls PersonDAO to save PersonDbo into db(more details in the code).
- UpdatePerson before updating person it checks if this persons already exists than similarly converts Person object to a database object PersonDbo and finally calls PersonDAO to update PersonDbo into db(more details in the code).
Now we have have only one existing test for this service TestPersonService as below with coverage 100%:
@Test
public void shouldSavePersonForFirstTime() {
Person modelPerson = TestData.createModelPerson();
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
@Test(expected = PersonRequiredFieldsMissingException.class)
public void shouldNotSavePersonWhenNameIsNull() {
Person modelPerson = TestData.createModelPerson();
modelPerson.setName(null);
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
@Test(expected = PersonRequiredFieldsMissingException.class)
public void shouldNotSavePersonWhenSurnameIsNull() {
Person modelPerson = TestData.createModelPerson();
modelPerson.setSurname(null);
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
@Test(expected = PersonRequiredFieldsMissingException.class)
public void shouldNotSavePersonWhenBirthdateIsNull() {
Person modelPerson = TestData.createModelPerson();
modelPerson.setBirthDate(null);
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
@Test(expected = PersonRequiredFieldsMissingException.class)
public void shouldNotSavePersonWhenSocialNumberIsNull() {
Person modelPerson = TestData.createModelPerson();
modelPerson.setSocialSecurityNumber(null);
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
@Test(expected = PersonNotExistException.class)
public void shouldFailToUpdatePersonWhenNotExisting() {
Person modelPerson = TestData.createModelPerson();
callPersonUpdateService(modelPerson, false);
}
@Test
public void shouldUpdatePersonWhenExisting() {
Person modelPerson = TestData.createModelPerson();
callPersonUpdateService(modelPerson, true);
}
As we can see this tests describes precisely how our PersonService should behave. So fare so good, we are testing all business logic classes( except Person and PersonDBO are just beans) beside PersonDAO which we cannot test with JUnit because of DB limitation. (For the moment lets forget about integration testing which can test with DB included since next post we will be all about that)
Change 1
Everything looks fine until one day a requirement change was needed for our service. The change was as simple as to add possibility to save addresses for each person. So basically now our Person can have many Addresses. So we change our Person and PersonDbo to have now respectively Address and AdressDbo.
- So basically we follow TDD in here and added a failing test first:
@Test
public void shouldSaveAddressedListPerEachPerson() {
Person modelPerson = createModelPerson();
ArrayList<Address> addressesModelList = new ArrayList<>();
modelPerson.setAddresses(addressesModelList);
Address addressModel = TestData.createAddressModel();
addressesModelList.add(addressModel);
assertPersonWasSavedWhenServiceCalled(modelPerson);
}
Indeed the test fail because right now PersonService is not able to save addresses.
- Continuing with TDD we have to make our test pass as soon as possible without thinking about design or anything else. The reason is because is easy and more efficient to do one thing at one time. Additionally since at this point we do not have a solution yet it may not be clear what design suits better. So PersonService was changed to PersonServiceChange1 to pass the test.
- Indeed we made our test green and following TDD now is time for us to refactor and improve our code. First thing to notice is that PersonService is becoming bigger from 90 lines to 130(PersonServiceChange1) and it is clear that will not scale when new entities like address are needed to be added to Person. Also is easy to notice that PersonService is not following Single Responsibility Principle as it is doing more than one thing like Converting Model objects to Database Objects, Validating… So we decide to separate converters to two different classes : PersonConverter and AddressConverter and change PersonService (PersonServiceRefactoredChange1)accordingly. After change we make sure the tests are green and indeed they are.
- Well now we think since two classes were created(PersonConverter and AddressConverter) it will be great to cover those with JUnit tests as well after all more coverage better it is and we need to make sure those classes work as expected. So we create TestPersonConverter and TestAddressConverter and happily go home.
The last step is the evil one and it doesn’t follow TDD. At this step we did not change the behavior but just improved/refactored Implementation Details and TDD says nothing about adding new tests after refactor step but rather simply that we need those existing tests green after refactoring. So what we basically did there is testing the Implementation Details and coupled our tests with those details. Also notice there is no code coverage increase after adding those tests.
I think : OK so what? I added a few more test.
Well I don’t think so lets see below how it goes… maybe you change your mind.
Change 2
This time a classical bug is found a NullPointerException. Below is the test which fails instead of handling a known exception like PersonRequiredFieldsMissingException:
@Test(expected = PersonRequiredFieldsMissingException.class)
public void testBug1345NullPointerWhenNullPerson() {
IPersonRepository mockPersonRepository = mockPersonRepository();
PersonServiceChange2 personService = new PersonServiceChange2(mockPersonRepository, new PersonConverter());
personService.savePerson(null);
}
PersonService(PersonServiceChange2) now looks as below:
private boolean areRequiredFieldFilled(Person person) {
return person.getBirthDate() != null &&
!StringUtils.isEmpty(person.getName()) &&
!StringUtils.isEmpty(person.getSurname()) &&
!StringUtils.isEmpty(person.getSocialSecurityNumber());
}
and we need to include also a check if Person is not null like below:
private boolean areRequiredFieldFilled(Person person) {
return person != null && person.getBirthDate() != null &&
!StringUtils.isEmpty(person.getName()) &&
!StringUtils.isEmpty(person.getSurname()) &&
!StringUtils.isEmpty(person.getSocialSecurityNumber());
}
With no problem now the test are all passing .We are good developers and follow the rule “Leave it cleaner than you found” so we decide to refactor a bit so will not have this kind of problems anymore. In order to avoid NullPointer in the future we decide to introduce new Java 8 Optional o our Converters. For compatibility reasons we did not introduce the change to PersonService. The reason we introduce to converters is because we notice the code below:
@Override
public Address convertToModel(AddressDbo addressDbo) {
if (addressDbo == null) {
return null;
}
Address address = new Address();
address.setPostalCode(addressDbo.getPostalCode());
address.setStreet(addressDbo.getStreet());
address.setHouseNumber(addressDbo.getHouseNumber());
address.setCountry(addressDbo.getCountry());
address.setCity(addressDbo.getCity());
return address;
}
Clearly we see that is not transparent that this method may return Null and so we think since this is not public API and it is just a refactor not change of behavior it will be great to change it.After all instead of NULL it will return just Optional and we need to change only few place the service is called. So we decide to do just that change to PersonConverter ,AddressConverter and PersonService(PersonServiceRefactorChange2).
After this change TestPersonServiceRefactorChange2 is passing but we see that PersonConverter ,AddressConverter are failing badly… They are failing in compile time and also in Runtime. We have two test classes and 6 methods in total to fix and we did not change anything functional just a refactor. So here we go: since we were testing Implementation Details and this details changed after refactor we have those test failing. Here we have only two entities and therefore only to tests but it can be easy to imagine a case when we have lets say 10 entities like Address and Person or even more. In that case the situation it will be even more messy and maybe that “Good Developer” will back up from his mission to “Leave it Cleaner than if found”.
So here we see that this tests instead of priming improvement they actually become an obstacle and pain to maintain without even offering more quality.
Test in Isolation
One of the suggestion of TDD is to test in isolation. And most of the time this is understand like testing only the class and nothing else more therefore we mock everything beside the current class.
This leads to hardly understandable tests.Test with many mocks which describe actually how the internal of the tests are working. Since this are mocks they will not complain when implementation will change so using many mocks may result to additionally out of synchronization code and also to many hours of updating them each time details change.
Well the problem in this case is that TDD does not mean to isolate the Tests from his internals or dependencies but rather to Isolate Tests from each other. So another JUnit test should not affect another one. One of the ways our test can interfere each other is by sharing same DB. So basically mocking DB and similar shared resource is in accordance with TDD. Otherwise we should use as much as possible real implementations and avoid mocking as much as possible.
If you find hard to use a real instance it means that probably you are testing an internal or your interfaces are not well defined. So it will better to first refactor and rethink the design.
In next post we will dig more deeply on how to solve the problem of mocking and integration tests as this is more of an architectural problem and how we think of our system layers.
Rule Are Made To Be Broken
If there is a rule out there is that “There is no absolute rule”. So basically applying one rule to everything is wrong as we need to flexible.
- Sometime the application is not that simple to understand and we feel the need to test his internals to understand how it works or behave. In this way we know how to use those internals or even maybe extend them. This cases is totally fine to add tests to explore those paths. But after we fix the problem we need to delete those tests. Wait a minute not so fast! Maybe someone else need those??? Can we just save someone else frustration??? Well yes but again we need to mark those tests as optional and clearly state that anyone can delete them if they become an obstacle for improvement or important refactor. And maybe you can document your experience instead of leaving the tests :).
- There is especially a case when we need to test HOW. This is the case with for example Cache. We really need to test if code is getting it from cache instead of lets say DB or HTTP. I would agree with such tests but anyway we all know that is better to switch to already build in solution like JPA since someone else is testing those things and cache is well one of the most difficult problem to solve especially on your own.
- There are also other cases when during the refactoring we produce a lot of of classes and feel the need to test them. I generally agree to test a part of them especially if we created a potentially public interface but otherwise I will skip them with no worry since anyway they are covered. Changing a requirement will flag parent test to fail anyway.
Conclusion
- Test What not How. Do not test Implementation Details.
- Do not add new test after TDD refactor step they are already covered.
- Avoid mocks as much as possible. Re design and lower dependencies instead.
- Feel free to break the rules with care when really needed.
Next post we will describe integration and end to end test and how I was wrong on designing the system architecture and tests as consequence.
References
- https://testing.googleblog.com/2010/12/test-sizes.html
- https://www.red-gate.com/simple-talk/dotnet/.net-framework/are-unit-tests-overused/
- https://testing.googleblog.com/2013/05/testing-on-toilet-dont-overuse-mocks.html
- http://codebetter.com/iancooper/2011/10/06/avoid-testing-implementation-details-test-behaviours/
- http://cdunn2001.blogspot.de/2014/04/the-evil-unit-test.html
- http://jacopretorius.net/2012/01/test-behavior-not-implementation.html
- https://news.ycombinator.com/item?id=7353767
- https://dannorth.net/introducing-bdd/
- http://tech.mybuilder.com/coupling-tests/
- http://www.duncannisbet.co.uk/hexagonal-architecture-for-testers-part-1
