DEV Community

Cover image for Things to avoid while testing your code
Ioannis Noukakis
Ioannis Noukakis

Posted on • Updated on

Things to avoid while testing your code

Testing, some have heard of it, some practice TDD religiously and some just add test for the sake of adding tests. Regardless of your motivations, be sure to void those common pitfalls when adding tests:

Test isolation

You may think its okay to test your DAO (Data Access Object) by inserting in the production database some data, make your test and then delete the test data. It is not ok. Not only you are modifying prod but you also are creating a concurrent mess where if two instance of the same test would run at the same time, the tests would fail RANDOMLY.

So don't do that and run your tests in isolation from each other:

  • Testing a database adapter? Use Testcontainers
  • Testing in the file system? Make a random temp directory to conduct each test in.
  • Need to mock a server? Choose a random port.

Assertions that are not assertions

If you see something like:

expect(dao.getAllClients().length).toBeGreaterThan(1)
Enter fullscreen mode Exit fullscreen mode

Then it is time to rethink the test. The flaw here is that you don't test WHAT the method is returning but only that it returns something. The correct way of testing your DAO is like this:

expect(dao.getAllClients()).toStrictEqual([{id: 1, name: "michel"}])
Enter fullscreen mode Exit fullscreen mode

"But I get those records from the production/testing database, I can't control in advance what the database is going to return"

Then my friend you are lacking some good old ISOLATION (see previous chapter).

Decouple!

If your tests take a very long time due to their dependance to a database / third party Rest API / having to start your web framework for each tests / etc then its time to decouple!

You don't have to tie all the business logic of your application to your database adapter. Instead hide everything behind an interface. That way you can replace the adapter by a fake one (test double) and mimic what the database would return without having to actually use one!

For example we have this service that returns client eligible for some corporation fidelity program (whatever):

export class ClientService {
    constructor(private readonly dbClient: MongodbClient) {
    }

    public selectEligibleClients(): Result {
        const clients = this.mongodbClient.clients.findAll();
        return clients.map(client => client.fidelityFactor > 0.8)
    }
}
Enter fullscreen mode Exit fullscreen mode

As you can see the MongoDbClient is tightly coupled with the service. Meaning if we want to test the service, we need a MongoDB instance. Now we are going to refactor this service to decouple the service from the MongoDB client:

export interface IClientServiceDatabasePort {
    getAllClients(): Array<Client>
}

export class ClientServiceMongodbDatabaseAdapter implements IClientServiceDatabasePort {
    constructor(private readonly mongodbClient: MongodbClient) {
    }

    getAllClients(): Array<Client> {
        return this.mongodbClient.clients.findAll();
    }
}

export class ClientServiceStubDatabaseAdapter implements IClientServiceDatabasePort {
    constructor(private readonly clientsToReturn: Array<Client>) {
    }

    getAllClients(): Array<Client> {
        return this.clientsToReturn;
    }
}

export class ClientService {
    constructor(private readonly dbClient: IClientServiceDatabasePort) {
    }

    public selectEligibleClients(): Result {
        const clients = this.dbClient.getAllClients();
        return clients.map(client => client.fidelityFactor > 0.8)
    }
}
Enter fullscreen mode Exit fullscreen mode

Now we can use the stub ClientServiceStubDatabaseAdapter in our tests and test the logic independently of the database and you can also test the ClientServiceStubDatabaseAdapter independently of the logic. I see this as an absolute win!

Too much irrelevant testing

You don't have to test EACH methods of your class (including what could be private but what you made public for the sake of testing), each little helper, etc.

If you followed the Single Responsibility Principle your class should expose only the relevant public API and keep its implementation details hidden.

"but how can we assert that the private methods of our classes work as intended?"

Well through the public API of course! Just because your test doesn't explicitly calls your private method it doesn't mean that it isn't tested. For example the following tests for a palindrome extractor (it extracts words from a string that can be red both ways):

it("Empty string is not a palindrome hence nothing is returned", () => {
    expect(palindromeExtractor.extract("")).toStrictEqual([])
})

it("Single letter is not a palindrome", () => {
    expect(palindromeExtractor.extract("a")).toStrictEqual([])
})

it("Double letter - same", () => {
    expect(palindromeExtractor.extract("aa")).toStrictEqual(["aa"])
})

it("Double letter - different", () => {
    expect(palindromeExtractor.extract("ab")).toStrictEqual([])
})

it("triple letter - palindrome", () => {
    expect(palindromeExtractor.extract("aba")).toStrictEqual(["aba"])
})

it("triple letter - asymmetric", () => {
    expect(palindromeExtractor.extract("abc")).toStrictEqual([])
})

it("multiple letter", () => {
    expect(palindromeExtractor.extract("sugus")).toStrictEqual(["sugus"])
    expect(palindromeExtractor.extract("train")).toStrictEqual([])
})

it("sentences", () => {
    expect(palindromeExtractor.extract("I eat sugus and it belongs to Anna"))
        .toStrictEqual(["sugus", "Anna"])
})
Enter fullscreen mode Exit fullscreen mode

And the implementation of the extractor:

export class PalindromeExtractor {
    extract(input: string): Array<string> {

        return input.split(" ")
            .filter(word => {
                if (word.length < 2) {
                    return false;
                }

                for (let i = 0, j = word.length - 1; i <= j; i++, j--) {
                    if (word[i].toLowerCase() !== word[j].toLowerCase()) {
                        return false
                    }
                }
                return true
            })
    }
}
Enter fullscreen mode Exit fullscreen mode

We can see that the only method of the class is well tested for the main cases (additional have been cut out for the sake of simplicity). Now lets say we want to do a bit of refactoring and move to its own method the code that checks if a word is a palindrome:

export class PalindromeExtractor {
    extract(input: string): Array<string> {

        return input.split(" ")
            .filter(word => PalindromeExtractor.isPalindrome(word))
    }

    private static isPalindrome(word: string): boolean {
        if (word.length < 2) {
            return false;
        }

        for (let i = 0, j = word.length - 1; i <= j; i++, j--) {
            if (word[i].toLowerCase() !== word[j].toLowerCase()) {
                return false
            }
        }
        return true
    }
}
Enter fullscreen mode Exit fullscreen mode

Now our code is a bit more readable. And, as you can see, we don't need to test isPalindrome since our tests already cover one word scenarios. So leave it like this. This private method is an implementation detail and it doesn't need to be tested. We already asserted that it works in the extractor's tests.

Conclusion

Now we have learned to:

  • Run our tests in isolation.
  • Make sure our assertions actually assert the results
  • Decouple our business logic from external libs such as db, filesystem, etc.
  • We only test what was meant to be tested.

That's all, until next time folks!

Top comments (0)