Every developer wants to promote code reuse. Generally, reuse is a good thing. It makes for more maintainable and more correct code.
There's one place where reuse shouldn't occur, though, and that's across the boundary between production and test code. Constants are a prime example of this. Let me show a concrete example to explain what I mean.
Let's assume you have this FizzBuzz code in your application in a FizzBuzz.java file:
String[] generate(int start, int end) {
String[] result = new String[end - start + 1];
for (int x = 0, number = start; number <= end; x++, number++) {
if (number % 15 == 0) {
result[x] = "FizzBuzz";
} else if (number % 3 == 0) {
result[x] = "Fizz";
} else if (number % 5 == 0) {
result[x] = "Buzz";
} else {
result[x] = String.valueOf(number);
}
}
return result;
}
And you write a few basic unit tests like so, in a FizzBuzzTest.java file:
@Test
void fizzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(3, 3);
assertEquals("Fizz", result[0]);
}
@Test
void buzzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(5, 5);
assertEquals("Buzz", result[0]);
}
@Test
void fizzBuzzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(15, 15);
assertEquals("FizzBuzz", result[0]);
}
Some developers here are tempted to refactor the solution into constants in FizzBuzz.java which then also show up in the test:
public class FizzBuzz {
public static final String FIZZ = "Fizz";
public static final String BUZZ = "Buzz";
String[] generate(int start, int end) {
String[] result = new String[end - start + 1];
for (int x = 0, number = start; number <= end; x++, number++) {
if (number % 15 == 0) {
result[x] = FIZZ + BUZZ;
} else if (number % 3 == 0) {
result[x] = FIZZ;
} else if (number % 5 == 0) {
result[x] = BUZZ;
} else {
result[x] = String.valueOf(number);
}
}
return result;
}
And in FizzBuzzTest.java:
@Test
void fizzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(3, 3);
assertEquals(FIZZ, result[0]);
}
@Test
void buzzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(5, 5);
assertEquals(BUZZ, result[0]);
}
@Test
void fizzBuzzTest() {
FizzBuzz fizzBuzz = new FizzBuzz();
String[] result = fizzBuzz.generate(15, 15);
assertEquals(FIZZ + BUZZ, result[0]);
}
Reasons not to do this
There are two key reasons why you shouldn't do this.
Tests change when production code changes
One reason not to do this is that it creates a situation where when production code is refactored, tests will unnecessarily get refactored alongside. Consider what would happen if you did an IDE refactoring to rename the constant FIZZ or move it to another class. You would also see the refactoring automatically touch the test code. That can be disconcerting because it introduces doubt as to whether or not the test is still accurate.
The ideal test code is code that doesn't change when production code is refactored, and the tests keep passing. This tells you your refactor is 100% safe. You should not usually have to change test code to change production code, and if you do, it should be because functionality actually is changing. Unfortunately, with tests that reference constants from production, this guarantee goes away because they are coupled together.
Tests will pass when the code is wrong
More importantly, tests that reference production constants can cause a situation where the code is actually wrong but the unit test passes anyway.
Consider the case where there's a misspelling in "Fizz" or even the wrong value, as here where it has the value "Buzz":
public class FizzBuzz {
public static final String FIZZ = "Buzz";
// . . .
The unit test will still pass, because the test is referencing against the same wrong FizzBuzz.FIZZ variable that the production code is referencing.
That's really scary and downright lethal in actual production code where the value of a constant often matters to some external interaction that the system makes. For example, a query parameter on a web service call or the name of a form field received from the front end.
Unit testing should always be the software equivalent to double-entry bookkeeping. This means that unit tests are meant to repeat values as a way to provide a second point-of-view on the correctness of a given piece of code. The unit test should check for the literal String value "Fizz" even if the production code uses a constant. This guarantees that the literal value expected from the constant is maintained.
Conclusion
For these reasons, I have concluded that test code should not reference constants in production code. Hope it helps.
Top comments (7)
A good post. When I'm implementing tests, I'm always trying to avoid any dependency on the code under test or to keep it as little as possible. Sometimes it is easier, e.g. when testing a web service or REST service. Sometimes it is not, e.g. when implementing unit tests or integration tests.
I'm trying not to use not only constants, but also any utilities the code under test depends on. The price is some code duplication. But to me it is more important that the dependency is reduced and thus the probability to find bugs increases.
And when I keep sometimes more dependency, it is not because I believe it does not affect the test, but because otherwise the implementation of the test or its maintenance would be too expensive.
Sounds like a great pragmatic approach!
Those are good reasons. There's a third one, readability. Like you say, re-use is good, and if done right you can argue it's readable. However, in a test you need to know EXACTLY what the test is doing right there and then. You don't want to have to go visit a constant to see what's being tested. Kent beck did a nice series on Test Desiderata awhile back, and one of the points was not abstracting tests.
Obviously abstracting the actual test code can be reasonable if it's highly re-used; it's a trade off on readability vs re-use. This is typically the case with end to end tests. "Login" functionality needs abstraction so that you don't repeate yourself over and over, and if "how" a Login changes you change it in one place. But that "Login" abstraction itself is test code, not production code.
Abstraction in tests can also be useful in some cases because you have to do common setup stuff that isn't really the centre of the test. Again this happens in component tests, e2e tests, but usually not unit tests.
I think the question for me here is around whether or not the constant is part of the API?
If it was I'd expect a test for the value of the constant, which would alleviate the second problem. And if the API changes shouldn't the tests change too?
For instance, I'd expect Integer.MAX_VALUE to be part of the test suite for Integer.
if the API changes shouldn't the tests change too? - no. Well, the tests should of course change. But the most important thing is, that this change should not be done automatically. The test should be changed manually, so that we have a chance to review the changes needed, to think about desired behaviour, about alternative.
Yes agree with this. Constants that are part of the API like Math.PI or Integer.MAX_VALUE should be tested.
Although you'd definitely think other tests would fail if these values were wrong.
I've just seen code recently where the only reason something was made "part of the public API" was actually just so it could be shared into the test code, not that it had a valid reason to be there.
You're right. If you don't intend to share it with your API users you shouldn't share it with your tests either.