EXPEDIA GROUP TECHNOLOGY — ENGINEERING
Healthy unit tests for healthy code bases
Unit tests: who doesn’t like writing those? A lot of people, apparently. But still, they are useful tools, so let’s try to make the best out of them.
In my time reviewing pull requests, I noticed a few common missteps that make unit tests ineffective, verbose, hard to maintain and a pain in the neck to write.
This article provides advice on avoiding some errors that may go unnoticed, especially among less experienced developers.
Note that some of the examples mentioned below in this blog are in Java and others in Javascript, but the principles are mostly interchangeable.
Reset the Mocks
Many unit tests require using some mocks to simulate external states and/or behaviour. Initializing those mocks often involves some common steps, and then configuring the state/behaviour needed for every single test.
But if you are not careful, your tests will start mixing things up. Consider this example:
const mockExternalStore = {
isValid: true
};
describe('UnderTest', () => {
it('is valid if external store is', () => {
const underTest = new UnderTest(mockExternalStore);
expect(underTest.isValid()).to.be.equal(true);
});
it('is not valid if external store is not', () => {
mockExternalStore.isValid = false;
const underTest = new UnderTest(mockExternalStore);
expect(underTest.isValid()).to.be.equal(false);
});
}These tests will pass, if they run in the order they are declared. But what happens if they are executed in reverse order? The second test will still pass, but the first one will now fail. That’s because that test has been written assuming that mockExternalStore.isValid == true, as in its initial state. But the second test alters the state of the mock, and the new state creeps into every test that runs after it.
Most test frameworks, both in Java and Javascript, provide you with some setup function usually named something like beforeEach. That is what you should use to make sure that each and every test starts from the same context. The example would then become:
let mockExternalStore;
describe('UnderTest', () => {
beforeEach(() => {
mockExternalStore = {
isValid: true
};
});
it('is valid if external store is', () => {
// I assume mockExternalStore.isValid == true is the agreed starting scenario, so don't assign it again
const underTest = new UnderTest(mockExternalStore);
expect(underTest.isValid()).to.be.equal(true);
});
it('is not valid if external store is not', () => {
mockExternalStore.isValid = false;
const underTest = new UnderTest(mockExternalStore);
expect(underTest.isValid()).to.be.equal(false);
});
}Be Careful with Exceptions
Sometimes we want to test that some illegal scenario leads to a specific exception being thrown. This can be done easily but is a good example of something that requires a little bit of care. Consider this example:
@Test
public void whenCallingForbiddenMethodThenThrowCustomException() {
try {
underTest.forbiddenMethod();
} catch (Exception e) {
assertTrue(e instanceof MyCustomException);
}
}That looks almost good: the test will fail if anything but a MyCustomException is thrown. But what if forbiddenMethod does not throw at all? The test will still pass, which is most likely not what you wanted.
You can fix this in two ways. Some test frameworks, such as JUnit, provide you with a specifically designed feature. That test can be fixed and simplified like this:
@Test(expectedException = MyCustomException.class)
public void whenCallingForbiddenMethodThenThrowCustomException() {
underTest.forbiddenMethod();
}If you don’t have such a feature available, or if you want to do specific checks on the thrown exception, you should remember to fail the test in case it does not follow the expected course:
@Test
public void whenCallingForbiddenMethodThenThrowCustomException() {
try {
underTest.forbiddenMethod();
fail();
} catch (MyCustomException e) {
assertTrue(e.getCause() instanceof AnotherCustomException);
}
}Prefer Explicit Test Data
Several tests can be replicated with different input parameters, in order to cover a wider array of regular and edge cases. Whether you do it manually or using some fancy @DataProvider, you must provide the set of input parameters.
Imagine having to test a hashing function based on a constant key.
public final String MY_KEY = "4e9aa2e2-0c29-4d01-a4fe-b464fd89ef74";
// Hashes input together with MY_KEY
public String hash(String input) {
...
}Your test might look like this:
@Test(dataProvider = "hashSamples")
public void hashingTest(String input, String expectedDigest) {
assertEquals(underTest.hash(input), expectedDigest);
}
@DataProvider
private Object[][] hashSamples() {
return new Object[][]{
{"input1", "E6d212KuLc0XvXsc"},
{"loooooonginpuuuuuuuuuut", "SCNb9HHscUPzCNHL"},
{"input+with-symbol$", "5ePJWwMwYwQBxLf9"},
{"", "aixwFwUnRmU1405D"},
{null, "4aHg05q4ftFzn7dX"}};
}But now you might think you don’t want the test to break if someone changes MY_KEY. You might be tempted to use some code to automate the generation of those parameters.
@Test(dataProvider = "hashSamples")
public void hashingTest(String input, String expectedDigest) {
assertEquals(underTest.hash(input), expectedDigest);
}
@DataProvider
private Object[][] hashSamples() {
return new Object[][]{
buildSample("input1", MY_KEY},
buildSample("loooooonginpuuuuuuuuuut", MY_KEY},
buildSample("input+with-symbol$", MY_KEY},
buildSample("", MY_KEY},
buildSample(null, MY_KEY}};
}
public Object[] buildSample(String input, String key) {
String digest = ... //Does this code look familiar?!?
return new Object[]{input, digest};
}Now you are basically replicating production code in the test, which defeats the purpose of the test itself.
This was a rather extreme example, but this can happen at a smaller scale unless you watch out for this.
Do Not Change Production Code for the Sake of Tests
In common practice, the one who develops the code also develops the tests. That leads to the wrong assumption that the two should be shaped together.
While it’s true that difficulty in writing tests often reflects an issue in the code design, the opposite is not always the case.
Consider this code:
public class BlackBox {
public doHouseCleaning() {
sweepTheFloor()
// ... some more code
doTheLaundry()
// ... some more code
doTheDishes()
// ... some more code
}
private sweepTheFloor() {
// ... a long method
}
private doTheLaundry() {
// ... a very long method
}
private doTheDishes() {
// ... an extremely long method
}
}Problem: testing all the nuances of
BlackBoxthroughdoHouseCleaning()is extremely hard.Solution: turn all private methods into package private, so you can test the smaller bits independently!
…wait, what? That does not sound right, does it? Most likely, you should rather spread the responsibilities over multiple classes. E.g.:
public class HouseCleaner {
public doHouseCleaning() {
broom.sweepTheFloor()
// ... some more code
washingMachine.doTheLaundry()
// ... some more code
dishWasher.doTheDishes()
// ... some more code
}
}
public class Broom {
public sweepTheFloor() {
// ...
}
}
public class WashingMachine {
public doTheLaundry() {
// ...
}
}
public class DishWasher {
public doTheDishes() {
// ...
}
}This is just an example of “fixing” the production code in order to make unit test development easier.
When you face a challenge in writing tests, try to identify the underlying issue, and fix that, rather than changing the production code with the test code in mind.
