Should I unit test for null values even if it can never happen?

Question

Lets say I have an event listener that is called whenever a person has changed their location. Here's some Java code.

public class LocationManager() {
//..
..
..//

public void updateLocationFromGPS(String userID, String locationName) {
    if(userID!=null && locationName!=null) {
        for(LocationChangeCallback callback : locationCallbacks)
        {
            callback.onLocationChange(userID, locationName, EventSource.GPS);
        }
    }

    }
}

And the callback...

public class Callback implements LocationChangeCallback {

public void onLocationChange(String userID, String locationName, EventSource eventSource) {
//DO SOME WORK


}

}

Where the event is sent and the callback is received are in two different components, hence I will need to unit test them both. Now, knowing that before I call the callback method, I do a null check on the values, and will only send the event if the values are not null.

Therefore, the values received in the callback will never be null. Is it best practice to add tests to check for null values (and change my code to pass accordingly) in the callback? If so, why?

Answer 1

Yes, it is. Because the unit tests are not only for the current version of the code. They are especially for all future versions and if somebody changes the code and does not know what s/he did, the test can fail and tell him that he did something wrong. So you can't say that this will never happen.

Also unit tests do not only test code, they are documentation too.

Answer 2

There are two questions here.

One, should you unit test for unexpected inputs? Yes. Any compilable inputs are a valid unit test, and nulls, boundary conditions, and other special values should regularly be tested. It is a public method, you do not know where it may be called from in the future or what someone else may do.

Two, if your unit testing finds some unexpected result, should you code defensively to avoid the unexpected result? Sometimes. If your function could produce a meaningful result from the given input, then you should modify it to do so. If your function could not produce a meaningful result (for example, if you are asked to open a file for reading when the file does not exist), then you should update your unit test to expect the error or exception. In either case you should end with a passing unit test.

Edit: Also, personally, one of the great benefits of unit testing is that you can produce edge and special cases on demand, even if they are difficult to find or appear impossible in the real world. There are billions of anecdotes about systems which were functioning properly for years until suddenly they weren't because someone changed some value somewhere off in a far corner of the database.

Answer 3

If it is part of the "defined interface" for onLocationChange that it must not be invoked with nulls, then there's not much point testing for its behaviour on nulls. You don't care what its behaviour on nulls is; if it is ever invoked with nulls, then the bug is in the caller.

From that, we can also conclude that you should absolutely be unit-testing all callers (including updateLocationFromGPS) such that you make sure they never pass nulls to onLocationChange. So declaring "nulls are just invalid, it's the caller's fault if I get them" generally increases your testing load, rather than decreasing it.

On the other hand, if there is some well-defined behaviour for onLocationChange when given nulls, then you should unit test its behaviour on nulls. Even if you never pass it nulls in your code base at the moment, and even if the well-defined behaviour is just throwing some particular exception.

Answer 4

After reading the answers I feel this is an important distinction to point out. 'Unit testing' and 'Checking parameters in your code' are two distinct things.

This is checking the parameters. My method checks there aren't null values before it does something invalid.

It documents assumptions you made in the method, enforces them, and fails fast. It's thing that's good to do with far more than null values. Also if they're distinct enough to be separately tested, their distinct enough to be reused separately.

So the answer to the body of the question (should you check parameters) is yes

public void Foo(string myString){ 
    Guard.IsNotNull(myString);
    return foo.ToUpper();
}

This is unit testing

public static void Foo(string myString){ 
    return foo.ToUpper();
}

[TestMethod]
[ExpectedException(typeof(NullReferenceException),"It went bang")]
public void TestWithNulls()
{
   SomeClass.Foo(null);
}

That wasn't terribly useful. Don't do that here. Unless you feel it's a key part of the functionality that the method goes bang.

Answer 5

Ok - so what is a Unit-Test designed for? Essentialy to test/verify that your method behaves like it should be according to the documentation/requirements.

So if a method is documented as "always returns 1" you would design unit tests to verify that whatever the inputs are, the function always returns 1.

Your Callback seems to be defined as "Does something XYZ. If Input is NULL will behave unexpected" If that is your requirement and the documentation of your Method, you don't have to write a Unit-Test for the inputs being NULL, because whatever your method does will be correct behavior according to the definition!

Because what would you test in your Unit test? Input = NULL -> Expected Result = ??? what would you assert, if there is no defined behavior in the specifications ?