This post is part of a series of blog posts about bad codes or bad practices I’ve come across. Note that this is my personal opinion and some of the things I mention could be debatable.
Post 1: Code review, why it matters
Post 2: Not including JS, CSS and static content files in project
Post 3: Let’s just not unit test our code
Post 4: Don’t pass dependencies into helper methods
Post 5: Inline SQL query statement in static helper class
In this post, I’d like to discuss some of the practices in relation to unit test that I consider as bad practices.
When I started with the last team, there was very little unit tests. The test coverage was about less than 7%. I questioned and pushed the team to start writing unit tests for their code. This turned out to be quite a difficult process as the existing team has never had a mindset about unit testing their code. Also, the application was a legacy application with a mix of web forms, MVC and various architectures and patterns used. There were lots of untestable code.
So I started with requesting changes on pull requests that were unit testable but did not have unit tests. The team was not on board with the idea and wanted to fall back to the old way so they started writing untestable code with lots of dependencies to avoid having to write unit tests. They started coding against concretion instead of abstraction.
i.e. They wrote these helper classes like below where it goes straight to the DB to retrieve data so it’s not unit testable
public static class ArticleHelper
{
public static Article GetArticleById(int id)
{
// Inline SQL query to get article using ADO.Net
}
}
// Which is then used in the ArticleService
public class ArticleService
{
public void SampleArticleAction(int id)
{
var article = ArticleHelper.GetArticleById(id);
// Other logic
}
}
instead of a more unit testable way
public interface IArticleRepository
{
Article GetById(int id);
}
public class ArticleRepository : IArticleRepository
{
public Article GetById(int id)
{
// Logic here to get article by id.
}
}
public class ArticleService
{
private readonly IArticleRepository _articleRepository;
public ArticleService(IArticleRepository articleRepository)
{
_articleRepository = articleRepository;
}
public void SampleArticleAction(int id)
{
var article = _articleRepository.GetById(id);
// Other logic
}
}
Another bad habit I see in the team is they would have a separate task for writing unit tests. Most of the time, someone else in the team other than the person that wrote the code would pick up the task and writing tests for it. The problem with this is if you don’t write unit tests as part of your task, how would you know your codes work? Also, this creates a bad mentality as developers would not care about writing unit tests or write clean code because someone else would write the tests for them and resulting in bad code. Another problem with this behaviour is that if there are bugs in the code because it’s untested, it would take a lot longer to fix. They would have to create a new task to investigate and fix the bug, then it would have to go through code review, deployment and QA. Finally, the biggest problem with getting someone else to write unit tests is it does not add much value to it. The developer writing the tests would only look at the logic in that code and testing it accordingly but would not identify if there’s any issue in the logic.
Lastly, the worst habit I’ve come across so far is writing tests without running them. I had tests for my code and when I ran the whole test suite to make sure I didn’t break anything, a few existing tests failed. After a bit of investigation, these tests would never have worked in the first place. The setup was all wrong, the inputs were also incorrect and I wasn’t quite sure what it was trying to test. Clearly whoever wrote these did not run them.
In conclusion, you should always have unit tests for your code. If anyone does not want to write test, simply ask them how they know their code works.