logo
Articleunittest

Unit testing common mistakes

Views: 872 Created: 2018-12-01 Read time: 6 minutes
Post Preview
Tags:

1)     Preface

Sometimes to know what to do and how to do it properly, we first need to get a good understanding of the pitfalls and caveats of a particular technique. It seems there is not much to know and take great caution when it comes to such a straightforward subject as unit testing. Nothing more misleading. If we do not follow specific standards and principles, we might quickly get ourselves in trouble and do more bad than good for the quality of our application. Dive in with me and let us go through the wall of shame of unit testing.

 

2)     Unit testing mistake #1: going for 100% coverage

Test coverage percentage has always been a conflict point between the team members. Some saying it should be between 60-80% while others say that the goal should always be 100%. In my opinion, the answer is: it depends.

Imagine that you are driving behind a car and you want to overtake it. Now with what speed you should perform the manoeuvre?

Unless you are Chuck Norris, given the information above, the answer is.. it depends. It depends on the speed the car is driving, on the traffic around you,  on the type of vehicle you are overtaking. Maybe it is a police car, or your boss in front of you and passing might not be the best idea at all.

The same idea applies to cover code with unit testing. Common sense defines the percentage of the coverage that you should aim for. There are some rules of thumb that you can (and should) follow and the coverage percentage will uncover itself for your particular application:

       Critical pieces of code (components) ought to be tested first with all the logical paths covered.

       Auxiliary and less error-prone code ought to be tested afterwards also with all the logical paths covered (if the deadline is closing fast we have at least the crucial units tested).

       Go for testing the happy path first. Leave the exceptional cases for last. 

So unless you are promised an extra bonus for testing every written line of code, covering methods listed below would be just a waste of time:


public Visit getVisitById(long id){
    return visitRepository.get(id);
}
public Address(String street, String houseNo, String city){
    this.street = street;
    this.houseNo = houseNo;
    this.city = city;
}

public Long getId() {
    return id;
}
public void setId(Long id) {
    this.id = id;
}

 

3)     Unit testing mistake #2: testing more than one logical path

In most of the situations, the method being tested has more than one logical path.

What you don't want to aim for is writing one test covering all those paths (or more than 1 path in general).

So given the example below:

 

SUT Code


private Patient patient;

public boolean isPatientLoggedIn(){
    if(patient != null){
        return true;
    }else{
        return false;
    }
}

 

Test Code


@Test
public void isPatientLoggedInTest() throws Exception{
    // Arrange
    UserManagedBean userManagedBean = new UserManagedBean();
    userManagedBean.setPatient(null);

    // Act
    boolean patientLoggedIn = userManagedBean.isPatientLoggedIn();

    // Assert
    assertFalse(patientLoggedIn);

    // Arrange 2
    userManagedBean.setPatient(new Patient());

    // Act 2
    patientLoggedIn = userManagedBean.isPatientLoggedIn();

    // Assert 2
    assertTrue(patientLoggedIn);
}

This test will obviously work and cover all the logical paths as intended. There are some problems, though:

       The name of the test cannot be anything more than just the name of the tested method. 
When the test fails, we look at the report, and we are not able to verify what exactly went wrong, we just know that there was some error while testing this particular method. If this is a common pattern in our system, then a ton of time is wasted for resolving unit testing issues. This should not take place.

       It is harder to analyse and maintain. There is a high chance that the method under test will be changed or extended at some point. That would probably cause more logical paths to be present and the need to add more variations to the test, which would clutter it up even more.


So to conclude.. the Single Responsibility Principle should be applied to all the code we write.. that includes the test code also. Otherwise, we would have a real irony in construction: creating tests to reduce technical debt, but causing technical debt to build up in the test code base at the same time. That leads to every automation artists nightmare: ignoring tests altogether.

 

Reference
Reference
generalart
Ignoring your tests

 

 

4)     Unit testing mistake #3: testing private methods

Too many or too long private methods are, generally a sign of lousy class design. You probably ended up with a bunch of public methods doing more than they should (have more than one single purpose). It will be hard to unit test them in a maintainable and readable way. If you follow the Single Responsibility Principle, your classes will usually have one or two specialised public methods (I would say, not larger than 15-25 lines of code) accompanied by one or two private methods each. Thanks to that, even though the public method uses some private implementation encapsulators, the method itself is not complex enough to perform the test cases only on it. No private is ever needed in that scenario.

 

Here are some key points about private methods that make them a really bad target for a unit test:

       Private methods are implementation details and testing them breaks the isolation.

       Other classes (clients) only see and care about public methods.

       They are highly prone to change.

       Require use of reflection, which makes the analysis way harder. It also makes them harder to maintain as you need to get the method with a raw String. If the method name changes.. we have a problem.

 

5) Unit testing mistake #4: skipping refactoring

Once the production method has been covered with tests, the final step is to perform the refactoring. Because we have written all the tests and the production method is guarded against any changes which would cause the test suite to fail, we can basically do any refactoring modifications to the design we can imagine. If we go too far. The suite will fail and remind us we have changed the logic. So let's assume we have a method given below and we have written all possible tests:

 

SUT Code


public static String formatTime( int hours, int minutes, int seconds)
{
    if(hours < 0 || hours > 23){
        throw new RuntimeException("Not proper hour value");
    }

    if(minutes < 0 || minutes > 59){
        throw new RuntimeException("Not proper minutes value");
    }

    if(seconds < 0 || seconds > 59){
        throw new RuntimeException("Not proper seconds value");
    }

    StringBuffer buf = new StringBuffer();
    if( hours < 10 ){
        buf.append( "0" + hours);
    }else{
        buf.append( "" + hours );
    }

    buf.append( ":" );

    if( minutes < 10 ){
        buf.append( "0" + minutes );
    }else{
        buf.append( "" + minutes );
    }

    buf.append( ":" );

    if( seconds < 10 ){
        buf.append( "0" + seconds );
    }else{
        buf.append( "" + seconds );
    }

    return buf.toString();
}

 

At this point, it would be a deadly sin if we left this production method as it is. We have every logical path covered, and it literally begs for some improvement. Let us never leave an opportunity like this behind.

 

Refactored Code


private static final String TIME_PART_SEPARATOR = ":";

public static String formatTime( int hours, int minutes, int seconds){
    validateHourPart(hours);
    validateMinutePart(minutes);
    validateSecondsPart(seconds);

    StringBuffer buf = new StringBuffer()
             .append(resolveTimePart(hours))
             .append(TIME_PART_SEPARATOR)
             .append(resolveTimePart(minutes))
             .append(TIME_PART_SEPARATOR)
             .append(resolveTimePart(seconds));

    return buf.toString();
}
private static String resolveTimePart(int timePart){
    if(timePart < 10){
        return "0" + timePart;
    }else{
        return "" + timePart;
    }
}

private static void validateHourPart(int hours){
    if(hours < 0 || hours > 23){
        throw new RuntimeException("Not proper hour value");
    }
}

private static void validateMinutePart(int minutes){
    if(minutes < 0 || minutes > 59){
        throw new RuntimeException("Not proper minutes value");
    }
}

private static void validateSecondsPart(int seconds){
    if(seconds < 0 || seconds > 59){
        throw new RuntimeException("Not proper seconds value");
    }
}

 

Now thanks to refactoring the public method are more clear and have better maintainability. Running the previously written test suite will ensure us that the logic has not changed and all tests still pass.

Refactoring is like an icing on the cake. Imagine a situation where you start to climb the highest hill in the area, and after all the effort, when you reach the peak, you immediately start to descend. Now, why would you want to do that.. stay a bit on the top, taking pride in what you accomplished enjoying the amazing view.

So covering the code with tests is the most important thing but along with refactoring, we end up with a tandem which just immensely boosts the quality of the code. Do not miss out!

 

6) Unit testing mistake #5: collaborators are not mocked

So far, we covered private methods, single paths, refactoring.. What about the interaction with collaborators of our SUT?

       Accessing the database

       Reading/Writing system files

       Connecting to a remote resource via network

       Invoking threads

Testing these, even though very tempting and justified at certain level, is not relevant in the unit testing context. Not even wanted to be exact. A unit test should test exactly, and only what the name implies, which is a unit. Accessing methods of the collaborators which are not trivial is certainly against that foundation.

Apart from that, the main strength of a unit test is.. speed. They are fast, and they should stay that way. Trying to call any of the resources from the list above would slow our test significantly. We do not want that. We want a slick and fast unit testing suite for our application so that it can be run in seconds by any developer at any given time during development.

What to do with invocations of these methods in that case you might ask. There is an answer to that as well, and it is called mocking. Get a head start on that subject with the following article:

 

Reference
Reference
unittest
Mockito stubbing

 

7)     Conclusion

 

Summary

 

We have seen how we should NOT approach our unit tests. We now know that accessing a database or a file do not have place here. We know that skipped refactoring is simply being lazy in the end and not finishing the job. We understand also that private methods are implementation details and should not be tested on their own under any circumstances. Now having all that in our mind right now let fire up our IDE and write some damn good unit tests!

Need more insight?
Repository
Repository
Glossary
Glossary
Tags:
Reference
You may also like:
tdd-common-mistakes
TDD Common Mistakes
basics-of-unit-testing
Basics of unit testing
test-method-naming
Test method naming
Comments
Be the first to comment.
Leave a comment