Once upon a time, when I started writing tests long time ago (actually, not that long, maybe a few years ago), I was a naive young man. I hate bugs so I write tests, and I wrote them according to my limited knowledge at that time.
Being naive and not exactly up to date with the references have a price. From each and every rejected PR review or regression bug, I've learned so much from my mistakes, and it made me realize that I had so much to improve. It is indeed unfortunate for me, having to learn through trials and errors, but it doesn't have to be unfortunate for you!
Say, fellow developers, should you feel that your test is not good enough, or your PRs have been rejected by the QA team too many times due to the lack of test quality, maybe you'll find this article useful. I am going to share with you the top five mistakes that I've made when writing tests, and why you should avoid them.
Before that, a disclaimer: the example code below is written in Javascript using Jest as the test framework. My focus is just specifically on Javascript so I can't comment much on the others, not sure if it can be applied. Also, these are just simplified examples, it does not represent actual use cases. Just to get the point across.
Alright. Right on to the example. Supposedly I was writing this class:
class Account {
constructor (initialBalance = 0) {
this.balance = initialBalance
}
deposit (money) {
this.balance += money
}
withdraw (money) {
this.balance -= money
}
}
Right now, the class is just simple. It has a way to deposit and withdraw an amount that would alter the balance. And my journey of writing the tests begins here.
1. Not keeping the test simple
First thing I wanted to test is the .deposit method. In my mind, the test has to be super specific, everyone else that reads the test wouldn't even need to see the actual code.
const account = new Account()
describe('Account class', () => {
describe('.deposit', () => {
test('Should increment the account balance by the amount', () => {
const increment = 200
const originalBalance = account.balance
account.deposit(increment)
expect(account.balance).toBe(originalBalance + increment)
})
})
})
The test looks good, right? It has the original balance, it has the amount to increment, and it asserts the original balance plus the increment. In fact, if I wanted to change the amount of the increment, I would only need to change the increment variable, and the test would still pass. That's it. Super easy.
Then came a new requirement. Every amount that is being deposited will be added 2% on top of the amount, as the incentive (don't ask me why, it's the PM...).
deposit (money) {
this.balance += (money * 1.02)
}
Hmm, yup, okay. So the test would be....
test('Should increment the account balance by the amount plus 2% incentive', () => {
const increment = 200
const originalBalance = account.balance
// PLEASE SEE TEH CODE FOR THE CLASS FOR REFERENCE
const incrementPlusIncentive = increment * 1.02
account.deposit(increment)
expect(account.balance).toBe(originalBalance + incrementPlusIncentive)
})
Oh gee, what is this monstrosity? My idea was to make it clear, but I ended up making it more complicated. Furthermore, I am duplicating the logic in the code to the test. That's not right.
In practice, test code should only be explicitly stating what you're testing (input -> output). No logic code should be there; it belongs to the code you're testing. Which is why, an improved version would be:
test('Should increment the account balance by the amount plus 2% incentive', () => {
account.deposit(100)
expect(account.balance).toBe(102)
})
There you go. Keep it simple. I am depositing 100, my balance is now 102. Is it according to the requirement? Absolutely! And that's what matters the most.
2. Not maintaining a clean state on each tests
My next quest is to write the rest of the test. .withdraw it is.
const account = new Account()
describe('Account class', () => {
describe('.deposit', () => {
test('Should increment the account balance by the amount plus 2% incentive', () => {
account.deposit(100)
expect(account.balance).toBe(102)
})
})
describe('.withdraw', () => {
test('Should decrement the account balance by the amount', () => {
account.withdraw(100)
expect(account.balance).toBe(2)
})
})
})
Hmm, yup, looks good. However, some of you might already notice it: there's a code smell. Why is that the tests are sharing one account instance? Wouldn't that make the order of the test matter, when it shouldn't? If we swap the order, it would definitely break. That is not right.
describe('Account class', () => {
describe('.deposit', () => {
test('Should increment the account balance by the amount plus 2% incentive', () => {
const account = new Account()
account.deposit(100)
expect(account.balance).toBe(102)
})
})
describe('.withdraw', () => {
test('Should decrement the account balance by the amount', () => {
const account = new Account()
account.withdraw(100)
expect(account.balance).toBe(-100)
})
})
})
By creating the account instance every test, it is ensured that the test begins with a clean slate. It can be modified as much as it wants, because it is contained in the scope of the particular test, and it is independent to each other. That way, the order of the test doesn't matter. Say, if we're using a test runner that runs in parallel and randomizes the order of the tests, it will still be passing just fine.
And by the way, there's beforeEach/afterEach (or setup/teardown) helper that we can also use to initialize and clean up every test suites, but it's rather complicated to explain here, so maybe for another article.
3. Not asserting the state properly
Next up, the project goes big, apparently there was some housekeeping going on, and now all code has to be commented, put it a proper file and whatnot.
/**
* Account class.
*/
class Account {
/**
* Constructor function.
*
* This sets the initial balance when initializing the instance.
*
* @param {Number} initialBalance
*/
constructor (initialBalance = 0) {
this.balance = initialBalance
}
/**
* Increment the balance by the given sum of the amount.
* An incentive of 2% of the amount will be added
* for each deposited amount.
*
* @param {Number} money
*/
public deposit (money) {
this.balance = (money * 1.02)
}
/**
* Decrement the balance by the given amount.
*
* @param {Number} money
*/
public withdraw (money) {
this.balance -= money
}
}
Alright, done. I didn't notice anything wrong (or did I? 😏 You'll find out soon enough). I checked out the Jest console and it says...
Account class
.deposit
✓ Should increment the account balance by the amount plus 2% incentive (5ms)
.withdraw
✓ Should decrement the account balance by the amount
Still passing, obviously. Duh. Committed, PR reviewed, CI build passed, merged and deployed. That was a fun Monday.
...but not really. Users are screaming that their balance is reset to the amount they're depositing. What is happening? How did that happen when the tests are passing?
I looked at my code, looked at the test, nothing seems wrong. Is it the initial balance? Because I didn't have a test for that (oops). So I go ahead and update the test as such:
describe('.deposit', () => {
test('Should increment the account balance by the amount plus 2% incentive', () => {
const account = new Account(100)
account.deposit(100)
expect(account.balance).toBe(202)
})
})
Lo and behold, not just the users, Jest is also screaming now (?)
● Account class › .deposit › Should increment the account balance
by the amount plus 2% incentive
expect(received).toBe(expected) // Object.is equality
Expected: 202
Received: 102
11 |
12 | account.deposit(100)
> 13 | expect(account.balance).toBe(202)
| ^
14 | })
15 | })
16 |
at Object.toBe (__tests__/index.test.js:13:31)
The bug appeared! This is exactly what users were reporting. Now the test actually failed. After looking at the code (and you can compare with the code from the beginning of this section), I noticed one tiny mistake:
deposit (money) {
// The plus was missing 🤮
this.balance += (money * 1.02)
}
Yup, there you go. A supposedly harmless refactoring went on to cause a bug, probably the plus was removed by accident. And the test couldn't catch it. I should have written it the proper way in the first place.
If the code is about value accumulation (not value assignment), it has to be tested in such a way that the previous value is accumulated with the value given. The previous assertion was sort of incomplete because it is just testing the value assignment.
// 🤔
describe('.deposit ❌', () => {
test('Should increment the account balance by the amount plus 2% incentive', () => {
const account = new Account() //... What's the previous value?
account.deposit(100) // Amount is 100
expect(account.balance).toBe(102) // Final value is 102...?
})
})
// 😎
describe('.deposit ✅', () => {
test('Should increment the account balance by the amount plus 2% incentive', () => {
const account = new Account(100) // Previous value is 100
account.deposit(100) // Amount is 100
expect(account.balance).toBe(202) // Final value is 202
})
})
To tie the knot, the constructor function has to be tested as well. This ensures the instantiation part is being covered properly (maybe if the constructor function has some logic, it can be asserted as well).
describe('constructor', () => {
test('Should set the initial balance when instantiated', () => {
const account = new Account(100)
expect(account.balance).toBe(100)
})
})
Maybe this section is rather specific, but the point is, always test the whole flow of the state (before/after, I/O), not just partially. At least that's what I've learned.
4. Not structuring the tests properly
I have received words from the QA team that I haven't been catching edge cases properly. Values in .deposit can be anything, and the error isn't intuitive enough.
Also, new requirement came: the account should be able to deposit more than one single amount, then make a sum out of it.
Fine. The .deposit code now looks like this:
/**
* Increment the balance by the given sum of the amount.
* An incentive of 2% of the amount will be added
* for each deposited amount.
* Only number is allowed, otherwise an error is thrown.
* Also, the number should be greater than 0.
*
* @param {Number[]} ...args
*/
deposit (...args) {
if (args.length === 0) {
throw new Error('Please provide at least one argument.')
}
const amount = args.reduce((total, value) => {
const number = parseInt(value, 10)
if (isNaN(number)) {
throw new Error('Please specify a number as the argument.')
}
if (number <= 0) {
throw new Error('Please specify a number greater than 0.')
}
return total + (number * 1.02)
})
this.balance += amount
}
...but the test isn't looking as good:
describe('.deposit', () => {
test('Should throw an error when no amount is given', () => {
const account = new Account()
expect(() => account.deposit()).toThrowError('Please provide at least one argument.')
})
test('Should throw an error when amount given is not a number', () => {
const account = new Account()
expect(() => account.deposit('a', 'b', 'c')).toThrowError('Please specify a number as the argument.')
})
test('Should increment the account balance by the sum of the amount plus 2% incentive, only when the amount is greater than 0 otherwise it should throw', () => {
const account = new Account(100)
account.deposit(100, 200)
expect(account.balance).toBe(406)
// ...but not less than 0!
expect(() => account.deposit(0, 400, -200)).toThrowError('Please specify a number greater than 0.')
})
})
Wow, the last part of the test was quite a mouthful. Whatever 🙄. Job's done, and tests are passing.
.deposit
✓ Should throw an error when no amount is given (4ms)
✓ Should throw an error when amount given is not a number (1ms)
✓ Should increment the account balance by the sum of the amount plus 2% incentive, only when the amount is greater than 0 otherwise it should throw (5ms)
However, QA team says that the test is a mess! It's hard to understand, and the last part of the test is doing too much. In general, it is better to split the tests into multiple contexts, so that there are layers of conditions to assert, and one test should simply do one thing based on the context.
An improved version would be:
describe('.deposit', () => {
describe('When no argument is provided', () => {
test('Should throw an error', () => {
const account = new Account()
expect(() => account.deposit()).toThrowError('Please provide at least one argument.')
})
})
describe('When the arguments are provided', () => {
describe('And the arguments are invalid', () => {
test('Should throw an error', () => {
const account = new Account()
expect(() => account.deposit('a', 'b', 'c')).toThrowError('Please specify a number as the argument.')
})
})
describe('And the arguments are valid', () => {
describe('And the arguments are less than zero', () => {
test('Should throw an error', () => {
const account = new Account()
expect(() => account.deposit(0, 400, -200)).toThrowError('Please specify a number greater than 0.')
})
})
describe('And the arguments are all more than zero', () => {
test('Should increment the account balance by the sum of the amount plus 2% incentive', () => {
const account = new Account(100)
expect(account.balance).toBe(100)
account.deposit(100, 200)
expect(account.balance).toBe(406)
})
})
})
})
})
The multiple layers of context is useful when the code grows even more complex. It's easier to add more contexts when it is already split as layers like that. For example, if I were to add a new validation (maybe there should be a maximum amount to deposit) and I was supposed to add a test for that, I know where to put them in the structure, nice and tidy.
The order of the layers are mostly my preference. I love seeing edge cases at the top and the actual logic at the bottom, kinda like how guards (or the actual validation) is written in the code.
And here's how it looks like on the Jest output:
.deposit
When no argument is provided
✓ Should throw an error (7ms)
When the arguments are provided
And the arguments are invalid
✓ Should throw an error (2ms)
And the arguments are valid
And the arguments are less than zero
✓ Should throw an error (2ms)
And the arguments are all more than zero
✓ Should increment the account balance by the sum of the amount plus 2% incentive (2ms)
Now I kinda have to agree with the QA team.
5. Not trusting the libraries you're using
The stakeholders say that there are hackers withdrawing money that weren't theirs from the account somehow. Due to that issue, the .withdraw function won't simply be deducting the balance; it has to go through some validation script magic so that it knows if it's being tampered by the hacker (I'm not sure how, this is just an example code).
/**
* Decrement the balance by the given amount.
* It is now using a validator from backend
* which I don't know how it works.
*
* @param {Number} money
*/
withdraw (money) {
const currentBalance = this.validateAndWithdraw(money)
this.balance = currentBalance
}
validateAndWithdraw (money) {
// This validator might throw an error if the transaction is invalid!!!
return superDuperValidatorFromBackend(money)
}
Due to the expensive cost of actually running it in Jest, I would rather mock the function that does the validation. As long as it won't throw me an error and give me the actual balance, it should be good to go.
describe('.withdraw', () => {
describe('Given a valid withdrawal', () => {
test('Should set the balance after withdrawal', () => {
const account = new Account(300)
// Override this function to avoid having to actually request from backend.
// It should just return the balance without any error thrown.
jest.spyOn(account, 'validateAndWithdraw').mockImplementationOnce(() => 200)
expect(() => account.withdraw(100)).not.toThrow()
expect(account.validateAndWithdraw).toHaveBeenCalledWith(100)
expect(account.balance).toBe(200)
})
})
})
I added not.toThrow() there so that I know when I call the .withdraw function, there is no error thrown, because I mocked it. Right? Right?
Wrong. -- QA team
Eventually, I learned that the tests that I write should only cover the business logic of my code. Testing whether it is thrown or not shouldn't be my test's responsibility, because the function implementation has been mocked by Jest, as I specified it in the test, so that the error won't be thrown. There is no need to assert if it should throw, because it will never throw!
...but how can you be so sure, though? -- My awkward testing skill
One can always check Jest's repository, the source code and how they're testing them, and if it's passing. There might even be the exact code, who knows. The point is, I have to trust the libraries I'm using, and it is their test responsibility to make sure that their code works, not mine. My test should focus on the actual logic on my code instead.
describe('.withdraw', () => {
describe('Given a valid withdrawal', () => {
test('Should set the balance after withdrawal', () => {
const account = new Account(300)
// Override this function to avoid having to actually request from backend.
// It should just return the balance without any error thrown.
jest.spyOn(account, 'validateAndWithdraw').mockImplementationOnce(() => 200)
account.withdraw(100)
expect(account.validateAndWithdraw).toHaveBeenCalledWith(100)
expect(account.balance).toBe(200)
})
})
})
That's it. Only business logic allowed.
And that concludes the end of my journey, for now. Who knows what the future (mistakes) holds...
Also, some of these mistakes might be obvious. But these points still stand. I just thought that I'd share. If you have any feedback on these suggestions, or maybe it wasn't such a mistake after all, let's discuss in the comment section below.
I hope you enjoy reading this article, and thank you!
Cover image by Jamie Street on Unsplash.
Top comments (10)
Love your writing style! It kept me interested all the way though. The lessons are great too!
Thanks! It means a lot since I just started writing two months ago, and I've kind of been shooting in the dark here. Glad that you like it. If you do have any feedback, do send them my way. Thanks, again.
I would argue that testing constructor that only assigns private initial value is unnecessary, seeing how you're testing methods that work with that value. After all, it doesn't matter what's going on inside a class if the output is correct.
You have a point. I've mentioned in the article, maybe if the constructor function has some logic, it can be tested and asserted as well. But true that, in the example I've given, testing the constructor itself might not mean that much, just putting it there.
Very nice write up. These lessons are the exact same one's I'm driving into my Quality Engineers.
I'll be sharing this article with them.
Cheer!
Thanks for the article! There's some good testing tips in here that I'll definitely be keeping in mind with my next project 👍
Glad to hear that!
Can relate to having those awkward testing skills 😅 Glad you learned along the way! And thanks for sharing them to us here 👍
Write tests first!
Nice article.
Wow, nice catch! Thanks. I've amended the article.