Should I test (duplicate) data, or only the behavior?

Question

From the design perspective, I am wondering should I test the data, especially if it's a generally known data (not something very configurable) - this can apply to things like popular file extensions, special IP addresses etc.

Suppose we have a emergency phone number classifier:

public class ContactClassifier {

    public final static String EMERGENCY_PHONE_NUMBER = "911";

    public boolean isEmergencyNumber(String number) {
        return number.equals(EMERGENCY_PHONE_NUMBER);
    }
}

Should I test it this way ("911" duplication):

@Test
public testClassifier() {
    assertTrue(contactClassifier.isEmergencyNumber("911"));
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

or (test if properly recognized "configured" number):

@Test
public testClassifier() {      
    assertTrue(contactClassifier.isEmergencyNumber(ContactClassifier.EMERGENCY_PHONE_NUMBER));
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

or inject "911" in the constructor,which looks the most reasonable for me, but even if I do so - should I wrote a test for the "application glue" if the component was instantiated with proper value? If someone can do a typo in data (code), then I see no reasons someone can do a typo in tests case (I bet such data would be copy-paste)

Answer 1

In my opinion the test should check behaviour and not the internal implementation. The fact that isEmergencyNumber verifies the number over constant declared in the class you're trying to test is verification over internal implementation. You shouldn't rely on it in the test because it is not safe.

Let me give you some examples:

Example #1: Someone changed EMERGENCY_PHONE_NUMBER by mistake and didn't notice. The second test will never catch it.

Example #2: Suppose ContactClassifier is changed by not very smart developer to the following code. Of course it is completely edge case and most likely it will never happen in practice, but it also helps to understand what I mean.

public final static String EMERGENCY_PHONE_NUMBER = new String("911");

public boolean isEmergencyNumber(String number) {
    return number == EMERGENCY_PHONE_NUMBER;
}

In this case your second test will not fail because it relies on internal implementation, but your first test which checks real word behaviour will catch the problem.

Answer 2

In my opinion that is not necessary to test this logic. The reason is: this logic is trivial for me. We can test all line of our code, but I don't think that is a good idea to do this. For example getter and setter. If we follow the theory to test all line of code, we have to write test for each of getter and setter. But these tests have low value and cost more time to write, to maintain. That is not a good investment

Answer 3

Writing a unit test serves an important purpose: you specify rules to be followed by the method being tested. So, when the method breaks that rule i.e. the behavior changes, the test would fail. I suggest, write in human language, what you want the rule to be, and then accordingly write it in computer language. Let me elaborate.

Option 1 When I ask ContactClassifier.isEmergencyNumber method, "Is the string "911" an emergency number?", it should say yes. Translates to

assertTrue(contactClassifier.isEmergencyNumber("911")); 

What this means is you want to control and test what number is specified by the constant ContactClassifier.EMERGENCY_PHONE_NUMBER. Its value should be 911 and that the method isEmergencyNumber(String number) does its logic against this "911" string.

Option 2 When I ask ContactClassifier.isEmergencyNumber method, "Is the string specified in ContactClassifier.EMERGENCY_PHONE_NUMBER an emergency number ?", it should say yes.

It translates to

assertTrue(contactClassifier.isEmergencyNumber("911")); 

What this means is you don't care what string is specified by the constant ContactClassifier.EMERGENCY_PHONE_NUMBER. Just that the method isEmergencyNumber(String number) does its logic against that string.

So, the answer would depend on which one of above behaviors you want to ensure.

Answer 4

What is the point in test data that you can test? That constant value is in fact constant value? It's already defined in code. Java makes sure that the value is in fact the value so don't bother.

What you should do in unit test is test implementation, if it's correct or not. To test incorrect behaviour you use data defined inside test, marked as wrong, and send to method. To test that data is correct you input it during test, if it's border values that are not well known, or use application wide known values (constants inside interfaces) if they're defined somewhere already.

What is bothering you is that the data, that should be well known to everyone) is placed in test and that is not correct at all. What you can do is to move it to interface level. This way, by design, you have your application known data designed to be part of contract and it's correctness checked by java compiler.

Values that are well known should not be checked but should be handled by interfaces of some sort to maintain them. Changing it is easy, yes, and your test will not fail during that change, but to avoid accidents with it you should have merge request, reviews and tasks that are associated with them. If someone does change it by accident you can find that at the code review. If you commit everything to master you have bigger problems than constants doubly defined.

Now, onto parts that are bothering you in other approaches:

1) If someone can do a typo in data (code), then I see no reasons someone can do a typo in tests case (I bet such data would be copy-paste)

Actually, if someone changes values in data and then continues to develop, at some point he will run clean-install and see those failed tests. At that point he will probably change/ignore test to make it pass. If you have person that changes data so randomly you have bigger issues, and if not and the change is defined by task - you made someone do the change twice (at least?). No pros and many cons.

2) Worrying about someone making a mistake is generally bad practice. You can't catch it using code. Code reviews are designed for that. You can worry though about someone not correctly using the interface you defined.

3) Should I test it this way:

@Test
public testClassifier() {
    assertTrue(contactClassifier.isEmergencyNumber(ContactClassifier.EMERGENCY_PHONE_NUMBER));
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

Also not this way. This is not test but test batch, i.e. multiple tests in the same method. It should be this way (convention-s):

@Test
public testClassifier_emergencyNumberSupplied_correctnessConfirmed() {
    assertTrue(contactClassifier.isEmergencyNumber(ContactClassifier.EMERGENCY_PHONE_NUMBER));
}


@Test
public testClassifier_incorrectValueSupplied_correctnessNotConfirmed() {
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

4) it's not necessary when method is properly named, but if it's long enough you might consider naming the values inside test. For example

@Test
public testClassifier_incorrectValueSupplied_correctnessNotConfirmed() {
    String nonEmergencyNumber = "111-other-222";
    assertFalse(contactClassifier.isEmergencyNumber(nonEmergencyNumber));
}

Answer 5

External constants as such have a problem. The import disappears and the constant is added to the class' constant pool. Hence when in the future the constant is changed in the original class, the compiler does not see a dependency between the .class files, and leaves the old constant value in the test class.

So you would need a clean build.

Furthermore tests should be short, clear to read and fast to write. Tests deal with concrete cases of data. Abstractions are counter-productive, and may even lead to errors in the test themselves. Constants (like a speed limit) should be etched in stone, should be literals. Value properties like the maximum velocity of a car brand can stem from some kind of table lookup.

Of course repeated values could be placed in local constants. Prevents typos, easy - as local - abstraction, clarifies the semantic meaning of a value.

However as cases in general will use constants maybe twice or three times (positive and negative test), I would go for bare constants.

Answer 6

I'd opt for

@Test
public testClassifier() {
    assertTrue(contactClassifier.isEmergencyNumber("911"));
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

as this doesn't test against something from the class under test that might be faulty. Testing with

@Test
public testClassifier() {      
    assertTrue(contactClassifier.isEmergencyNumber(ContactClassifier.EMERGENCY_PHONE_NUMBER));
    assertFalse(contactClassifier.isEmergencyNumber("111-other-222"));
}

will never catch if someone introduces a typo into ContactClassifier.EMERGENCY_PHONE_NUMBER.