Sources Contact Advanced Search Tutorials

An Interest In:

Web News this Week

Search Archive

Some of Our Sources

View All Sources

Help Webnuz

Referal links:

July 6, 2019 07:09 pm GMT

A Series of My Unfortunate Mistakes (When Writing Tests)

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 on 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 in 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 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 deploy. 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 multiple amounts, then make a sum out of it.

Fine. The .deposit code now looks like this:

  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  }

...and 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. Anyway, 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 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), I would just add them under And the arguments are valid describe section.

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. 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)        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 so that I don't have to test them again. 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)        jest.spyOn(account, 'validateAndWithdraw').mockImplementationOnce(() => 200)        account.withdraw(100)        expect(account.validateAndWithdraw).toHaveBeenCalledWith(100)        expect(account.balance).toBe(200)      })    })  })

There you go. And that would be the end of my journey. Who knows what the future (mistakes) holds..

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.

Original Link: https://dev.to/briwa/a-series-of-my-unfortunate-mistakes-when-writing-tests-h8m

Share this article:

View Full Article

Dev To

An online community for sharing and discovering great ideas, having debates, and making friends

More About this Source Visit Dev To