Ryan Lue
Ryan Lue

Reputation: 997

How to unit test a class that depends heavily on other classes?

My understanding is that unit testing should test classes in isolation, focusing on granular behavior and substituting objects of other classes using doubles/mocks wherever possible. (Please correct me if I'm wrong here.)

I'm writing a gem with a class called MatchList. MatchList::new takes two arguments, each an instance of another class called MatchPhrase. MatchPhrase contains some behavior that MatchList depends heavily on (i.e., if you feed anything other than a MatchPhrase to MatchList::new, you're going to get a bunch of "undefined method" errors).

My current (naive?) test setup uses let statements to assign variables for use in my examples:

let(:query)      { MatchPhrase.new('Good Eats') }
let(:candidate)  { MatchPhrase.new('Good Grief') }
let(:match_list) { MatchList.new(query, candidate) }

How do I write this unit test? Am I right in thinking it should be done without invoking the MatchPhrase class? Is that even possible?


For reference, here is what the MatchList class looks like:

class MatchList < Array
  attr_reader :query, :this_phrase, :that_phrase

  def initialize(query, candidate)
    super(query.length)
    @query = query
    @this_phrase = query.dup
    @that_phrase = candidate
    find_matches until none?(&:nil?)
  end

  private

  def find_matches
    query.each.with_index do |this_token, i|
      next unless self[i].nil?
      that_token = this_token.best_match_in(that_phrase)
      next if that_token.match?(that_token) &&
              this_token != that_token.best_match_in(this_phrase)
      self[i] = this_token.match?(that_token) ? that_token : NilToken.new
      this_phrase.delete_once(this_token)
      that_phrase.delete_once(that_token)
    end
  end
end

Upvotes: 6

Views: 2649

Answers (3)

fylooi
fylooi

Reputation: 3870

Mocking should be done for legitimate reasons and not as a matter of principle.

If there is only one collaborator class and your primary class is heavily coupled to it, mocking out the collaborator as a matter of principle may result in more fragility than benefit as the mock will not reflect the behavior of the collaborator.

Mocks and stubs are good candidates when you can reason against the mock's interface instead of an implementation. Let's ignore the existing code and look at the interfaces in use here:

  • MatchList.new takes a query and candidate
  • query is an Enumerable containing objects which implements best_match_in?(something)
  • The objects in query also implement delete_once(something)
  • candidate also implements delete_once(something)
  • best_match_in? returns something that implements match? and best_match_in?

Looking at the interfaces in use, MatchList appears to rely pretty heavily on the implementation of the query and candidate objects. Smells like a case of feature envy to me. Perhaps this functionality should reside within MatchPhrase instead.

In this case, I would write unit tests using actual MatchPhrase objects with a note to refactor this code.

Upvotes: 1

Marko Avlijaš
Marko Avlijaš

Reputation: 1659

My understanding is that unit testing should test classes in isolation, focusing on granular behavior and substituting objects of other classes using doubles/mocks wherever possible. (Please correct me if I'm wrong here.)

In my understanding this is not true. Using doubles/mocks has advantages and disadvantages.

Advantage is that you can take a slow service like database, email and mock it with fast performing object.

Disadvantage is that object that you are mocking is not "real" object and might surprise you and behave differently than real object would.

That's why it's always better to use real objects if practical. Only use mocks if you want to speed up your tests or if it leads to much simpler tests. Even then have one test using real object to verify that it all works. This is called integration test.

Considering your case:

let(:query)      { MatchPhrase.new('Good Eats') }
let(:candidate)  { MatchPhrase.new('Good Grief') }
let(:match_list) { MatchList.new(query, candidate) }

There is really no advantage to mock query or candidate.

Upvotes: 2

sameera207
sameera207

Reputation: 16629

Your understanding of using test part is correct. Its about focusing on granular behavior. E.g individual methods.

However to test the individual methods, try using doubles/mocks is not advisable. Marko ^^ has outlined the advantages / disadvantages of the mocks. I personally prefer not to not to use doubles/mocks as much as I can.

Its always a balance between the speed of your tests and the objects you create.

Before moving to doubles/mocks, its a good idea to see if you can write your test without saving the values to the DB. Like you have done before. That is faster than saving and retrieving the values from the DB.

One more thing is, private methods and not generally unit tested. This is with understanding of, the caller of your private method will have a unit test.

let(:query)      { MatchPhrase.new('Good Eats') }
let(:candidate)  { MatchPhrase.new('Good Grief') }

it 'check your expectation' do
  expect(MatchList.new(query, candidate).query).to <check the expectation>
end

However I will re-evaluate the following points.

1 - do u want to call find_matches from initializer

2 - Let find_matches to return a value and then assign it to the @query variable (so that its easy to test the method with a return value)

3 - rename the query param in init to something else (just to avoid the confusion)

And the golden rule is If its hard to test (specially unit test), maybe your are doing something wrong.

HTH

Upvotes: 1

Related Questions