Unit testing enhances the software quality. It might not be written for private projects but it should be written in a good way for public software. There are many good things that we should follow to write clear, readable and nice unit tests. Many articles on various blogs explain bad practices but I also want to introduce them with examples. These are what I have experienced from my career. Some of them might be good at first but they are going to be annoying as the software grows.
Error message that tells nothing
The first one is mild. When we write a unit test we use a function provided by a framework to compare the actual value with the expected value. However, sometimes… sometimes those functions are not used. Even though there are lots of existing unit tests in the repository and a developer has ever written unit tests in another module in a good manner, it seems to happen.
Following test code of course works if the production code is correct but it throws an error if the implementation is incorrect. Its message tells nothing.
// BAD
it("should get correct object", () => {
const result = getObject();
if(result.prop1 === 1){
throw new Error("Unexpected error.");
}
});It may be ok if the number of properties to be checked is 1 but the error message should be clear. Otherwise, we have to run the code with debug mode in the following case. We have to know which property is wrong.
// BAD
it("should get correct object", () => {
const result = getObject();
if(result.prop1 === 1 && result.prop2 === 2){
throw new Error("Unexpected error.");
}
});To improve this unit test, let’s use one of the useful functions provided by a framework. Following code tells you which property is wrong if the actual value is incorrect.
// GOOD
it("should get correct object", () => {
const result = getObject();
expect(result).to.deep.equal({prop1: 1, prop: 2});
});The following test is for Node-RED. This is not a good test. When the actual value is different from the expected value it will be timeout. expect the function throws an error but it is not caught by anyone. We can’t know why the error occurs.
// BAD
it("should send correct data", () => {
const expected = getObject();
helper.load(requiredNodes, flow, () => {
const inNode = helper.getNode("in-node-id");
const outNode = helper.getNode("out-node-id");
outNode.on("input", (msg: any) => {
expect(msg.payload).to.equal("test-data");
});
inNode.send({ payload: "test-data" });
});
});This is what I actually saw. To show an error message, we need to pass an argument to done function but the error message is nonsense.
// BAD
it("should send correct data", (done) => {
const expected = getObject();
helper.load(requiredNodes, flow, () => {
const inNode = helper.getNode("in-node-id");
const outNode = helper.getNode("out-node-id");
outNode.on("input", (msg: any) => {
if (msg.payload === "test-data") {
done();
} else {
done(Error("Unexpected Error"))
}
});
inNode.send({ payload: "test-data" });
});
});The code should be the following. Use the right function and specify the incoming error variable.
// GOOD
it("should send correct data", (done) => {
const expected = getObject();
helper.load(requiredNodes, flow, () => {
const inNode = helper.getNode("in-node-id");
const outNode = helper.getNode("out-node-id");
outNode.on("input", (msg: any) => {
try { // change to try-catch
expect(msg.payload).to.equal("test-data");
done();
} catch(e) {
done(e); // use incoming error
}
});
inNode.send({ payload: "test-data" });
});
});Function to execute multi tests
It may be a good thing to create a function to execute multiple tests if there are many cases that need the same tests. If we want to put them together it should be well organized. Let’s see an example.
We have many services. They can be used after a user pays for it and the user needs to change the configuration. Therefore, we need to write the following 4 tests.
function executeServiceTest(service: string, data: string) {
describe(`Service tests for ${service}`, () => {
let obj;
beforeEach(() => {
obj = createServiceObj(service);
});
it("should return decorated value when isPaid and serviceEnabled are true", () => {
obj.isPaid = true;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.equal(`---${data}---`);
});
it("should return empty when isPaid is true and serviceEnabled is false", () => {
obj.isPaid = true;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is true", () => {
obj.isPaid = false;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is false", () => {
obj.isPaid = false;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
});
}OK. It looks cool. We can write the same test for different services with minimal code.
executeServiceTest("service1");
executeServiceTest("service2");
executeServiceTest("service3");But what if we want to add some tests into it only for service2? If we add some tests there its change affects other service tests. We can create another describe() class but then, its test group is different.
What if we want to run only one test case because it takes long for each test? Do we want to comment out in two places, Function call and executeServiceTest function?
I had these issues in my project. It takes more than 500 ms for each and I didn’t want to run them all. Even if we add only keyword 3 test cases are executed because there are 3 function calls.
I want to change this structure to the following.
function executeServiceTest(obj: object, data: string) {
it("should return decorated value when isPaid and serviceEnabled are true", () => {
obj.isPaid = true;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.equal(`---${data}---`);
});
it("should return empty when isPaid is true and serviceEnabled is false", () => {
obj.isPaid = true;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is true", () => {
obj.isPaid = false;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is false", () => {
obj.isPaid = false;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
}
describe("Service test", ()=> {
let obj;
describe("Service test for service1", ()=> {
beforeEach(() => {
obj = createServiceObj("service1");
});
executeServiceTest(obj, "test-data1");
});
describe("Service test for service2", ()=> {
beforeEach(() => {
obj = createServiceObj("service2");
});
executeServiceTest(obj, "test-data2");
it("write additional test here", () => {
// do something
});
});
describe("Service test for service3", ()=> {
beforeEach(() => {
obj = createServiceObj("service3");
});
executeServiceTest(obj, "test-data3");
});
});describe call is extracted to outside so we can easily add additional tests there. If we can replace createServiceObj with stub object, we can easily handle this case because it is an interface and the stub can always return the same value. However, it is sometimes not possible. In my case, it was a Node-RED node test. Test input data goes through a Node-RED flow. Therefore it is not possible to replace the intermediate process if the process is composed of multiple nodes. The example above was introduced to write the same tests for the different data sources. It was good at first but we needed to add additional tests for some data sources. The 4 tests needed to be split.
I think the best way is to write tests one by one without creating a function to execute multiple tests. It’s clear what the test does. If we want to make the test easy, create a function that does setup and executes only one test. Specify input and expected value to the function in each test case. The example above is very simple so we don’t need to create it though. If it’s necessary the code looks something like this.
function runTest(args: { done?: Mocha.Done, input: unknown, expected: unknown }){
let error;
try
{
// write actual test process
// Arrange - preparation step
const arrange = prepareSomething(input);
// Act - call a function that you want to test
const result = doSomething(arrange);
// Assert - verify the result
expect(result).to.equal(args.expected);
} catch(e) {
error = e;
} finally {
if(args.done){
args.done(error);
}
}
}
it("should be 1 for input 1", () => {
const input = 1;
const expected = 1;
runTest({input, expected});
});
it("should be 4 for input 2", () => {
const input = 2;
const expected = 4;
runTest({input, expected});
});
it("should be 9 for input 3", () => {
const input = 3;
const expected = 9;
runTest({input, expected});
});We don’t need to add done property unless the target function is called in an async callback.
Conditional statements to determine whether the test should be executed
I have seen this a few times in my project. This can happen if there is a function to execute multiple tests as I showed above or if test data is in a file that is explained later. I added one test at the top of the function.
function executeServiceTest(obj: object, data: string) {
if(obj.databaseServiceEnabled) {
it("should have databaseId", () => {
const result = obj.getDatabaseId();
expect(result).to.equal(data.databaseId);
});
}
it("should return decorated value when isPaid and serviceEnabled are true", () => {
obj.isPaid = true;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.equal(`---${data}---`);
});
it("should return empty when isPaid is true and serviceEnabled is false", () => {
obj.isPaid = true;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is true", () => {
obj.isPaid = false;
obj.serviceEnabled = true;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
it("should return empty when isPaid is false and serviceEnabled is false", () => {
obj.isPaid = false;
obj.serviceEnabled = false;
const result = obj.getDecoratedData(data);
expect(result).to.be.empty;
});
}The test is executed if the database service is enabled. It looks good at first but the potential bug is lurking there. Are you able to guarantee that all developers don’t forget to set the property in the test? Has a new team member already known that property? Are you sure?
If this condition is introduced by someone else at some time you should deny the PR because some of the tests might not be executed due to the missing property. It is hard to find the problem if something goes wrong because all test results become green. It took me a while to find the reason. The necessary properties were not defined in some tests defined in a file. If there are a bunch of tests it is hard to find that the target tests are not executed.
The condition is actually unnecessary if we write those tests separately. Unit test should be clear enough. The fewer conditional statements there are, the better the test is. Avoid adding conditional statements in a unit test. Choose clarity rather than less code for unit test.
Putting test data into a file
You might not want to write many test codes and might want to start adding the test data into a file like this below.
{
{
"title": "should be 'Learning Unit Test' when service is service1 and data is 1",
"input": {
"service": "service1",
"data": 1
},
"expected": "Learning Unit Test",
}
{
"title": "should be 'Learning TypeScript' when service is service2 and data is 1",
"input": {
"service": "service2",
"data": 1
},
"expected": "Learning TypeScript",
}
{
"title": "should be 'Learning Web Design' when service is service1 and data is 3",
"input": {
"service": "service1",
"data": 3
},
"expected": "Learning Web Design",
}
}Let’s write a function for the tests.
function runTest(title: string, input: object, expected: string) {
it(title, () => {
const instance = getService(input.service);
const result = instance.getCourse(input.data);
expect().to.equal(expected);
});
}
const testData = require("test-data.json");
testData.forEach((test) => runTest(test.title, test.input, test.expected));This might be good at first but it is not flexible, isn’t it? What if we need to add an additional property for ‘Learning TypeScript’? Let’s consider the following case that we add type property.
{
{
"title": "should be 'Learning JavaScript' when service is service2, data is 1 and type is JavaScript",
"input": {
"service": "service2",
"data": 1,
"type": "JavaScript"
},
"expected": "Learning JavaScript",
},
{
"title": "should be 'Learning TypeScript' when service is service2, data is 1 and type is TypeScript",
"input": {
"service": "service2",
"data": 1,
"type": "TypeScript"
},
"expected": "Learning TypeScript",
}
}To address this change we need to adjust the test.
function runTest(title: string, input: object, expected: string) {
it(title, () => {
const instance = getService(input.service);
const result = instance.getCourse(input.data, input.type); // add input.type here
expect(result).to.equal(expected);
});
}
const testData = require("test-data.json");
testData.forEach((test) => runTest(test.title, test.input, test.expected));This seems to work but wait. The return type of getService is now something like this. type property is nullable even though the service2.getCourse definitely requires the value and other services don’t require it.
type GetCourse = (data: number, type?: string) => string;Then, should we change the code like this?
function runTest(title: string, input: object, expected: string) {
it(title, () => {
const instance = getService(input.service);
const getCourse = () => {
if(input.type){
return instance.getCourse(input.data);
} else {
return instance.getCourse(input.data, input.type);
}
};
const result = getCourse();
expect(result).to.equal(expected);
});
}
const testData = require("test-data.json");
testData.forEach((test) => runTest(test.title, test.input, test.expected));Ahhhhh, unnecessary conditional statements come again. The test code is not so bad at the moment because I just added only one exceptional case that made a small change. However, it will definitely be more complicated as the software grows. The small change matters!
Another problem to use a file is that it’s not possible to define our own object there. We can define the boolean, number, string in a JSON file. The objects that we define there is just an object even if it has the same properties as our own object/class. It means we need to cast the object in our unit test. If there are different objects in different tests we need to add conditional statements again to create a proper object.
Try to avoid using conditional statements in a unit test.
End
How do you think the unit test examples above? Do you write such a code? Have you seen a similar code in your projects? Do you disagree with some of them?
Comments