I am a big fan of writing tests but sometimes no tests are better than weak tests. Without tests, you know that you have to test everything manually but with a weak test, you are deluding yourself and people around you that everything is ok.

How to detect if a test is weak and we can’t really depend on it? Here are the most common mistakes made during tests creation.

We will write a simple class and a weak test and analyze it:

module Users
  class NameService
    def initialize(user)
      @user = user
    end
  
    def name
      if user.name.present?
        user.name
      else
        ::ExternalAPI.new(user).get_name
      end
    end
  
    private
    attr_reader :user
  end
end

Our class returns user name if available otherwise call some external API and get user name from an online profile. Let’s write now really poor tests:

require 'spec_helper'

describe Users::NameService do
  describe '#name' do
    it 'returns name' do
      user = User.create!(name: 'Mike Black')
      service = described_class.new(user)
      
      expect(service.name).to eq(user.name)
    end
    
    it 'returns name' do
      user = User.create!(name: nil)
      service = described_class.new(user)
      
      expect(service.name).to eq('Mike Jr Black')
    end
  end
end

So now what do you think: what is wrong here?

1. Incomprehensible documentation

Run this spec with --format documentation flag and you will notice that without looking at the class and test code you won’t be able to tell what this method really does. Of course, it returns name but you don’t know anything else. Tests are one of the best sources of knowledge about given app no matter if you are junior or a senior developer so it’s important to write meaningful example descriptions. Let’s fix it:

describe Users::NameService do
  describe '#name' do
    it 'returns user name if the user has a name' do
      user = User.create!(name: 'Mike Black')
      service = described_class.new(user)
      
      expect(service.name).to eq(user.name)
    end
    
    it 'returns user name from API if the user does not have a name' do
      user = User.create!(name: nil)
      service = described_class.new(user)
      
      expect(service.name).to eq('Mike Jr Black')
    end
  end
end

Do you see the difference? You can just run specs with documentation flag and you don’t have to look at the code to know what is going on there.

2. No isolation

We want to test our name service but we also call code inside ExternalAPI#get_name class – this does not make sense as in unit tests we want to isolate a little piece of code (usually a method) and test only it. We don’t want anything else to have impact on it. Let’s assume that your class works well but there is a error in ExternalAPI#get_name – our test would fail and we would have to fix two tests, for Users::NameService#name and ExternalAPI#get_name. To avoid it we can use stubs:

describe Users::NameService do
  describe '#name' do
    it 'returns user name if the user has a name' do
      user = User.create!(name: 'Mike Black')
      service = described_class.new(user)
      
      expect(service.name).to eq(user.name)
    end
    
    it 'returns user name from API if the user does not have a name' do
      user = User.create!(name: nil)
      external_api = instance_double(ExternalAPI, get_name: 'Mike Jr Black')
      allow(ExternalAPI).to receive(:new).with(user).and_return(external_api)
      
      service = described_class.new(user)
      
      expect(service.name).to eq('Mike Jr Black')
    end
  end
end

Now our test is isolated and it’s much faster than before. Even if there would be an error inside API class our test would not fail because its implementation is ok.

3. Useless database usage

We don’t really need to hit the database to write this test. Let’s use stub again and speed up our tests:

describe Users::NameService do
  describe '#name' do
    it 'returns user name if the user has a name' do
      user = instance_double(User, name: 'Mike Black')
      service = described_class.new(user)
      
      expect(service.name).to eq(user.name)
    end
    
    it 'returns user name from API if the user does not have a name' do
      user = instance_double(User, name: nil)
      external_api = instance_double(ExternalAPI, get_name: 'Mike Jr Black')
      allow(ExternalAPI).to receive(:new).with(user).and_return(external_api)
      
      service = described_class.new(user)
      
      expect(service.name).to eq('Mike Jr Black')
    end
  end
end

Now we didn’t hit the database or any external service. Our test are isolated and super fast. Of course we can do further refactoring and move User model instance stub to the separated method but it’s not necessary in this example (but it’s nice of course).

4. Testing private methods

I didn’t cover this case on our example but it’s also a common mistake. We don’t have to test private methods because we already test public methods and their results produced also by private methods. If you feel that you should test your private methods then consider splitting your code into the smaller classes and test them separately. The best way to avoid such cases is to use single responsibility principle when you design methods.

Download free RSpec & TDD ebook

Do you want to earn more or jump to the next level in your company? Do you know that testing skills are one of the most desired skills? There is only first step: start testing and do it right. My ebook can help you. Subscribe to the newsletter to get a free copy of the book.

2 Comments

  1. Rob Schlüter

    Does this code actually run? To me it looks like the `serive` in `serive.name` just appears from nothing. But I don’t speak Ruby so may miss some details of the code.

Leave a Comment

Your email address will not be published. Required fields are marked *