Are there any glaring issues with the way I write and test my Ruby classes?

Question

After fumbling around with Ruby for a few weeks I've fallen into a coding pattern that I'm comfortable with. When I create a new class and that class has dependencies, I add a method initialize_dependencies that does the job of creating them. That function is then executed from the constructor.

When I write a test for this class, I actually test a subclass where the initialize_dependencies function has been overridden and the dependencies replaced with stubs or mocks. Here is an example of such a test (with most tests and test data removed for brevity):

require "address_kit/entities/street_address"

module AddressKit
  module Validation
    describe FinnValidationDriver do
      # Subject returns hard coded results from finn client.
      subject {
        Class.new(FinnValidationDriver) {
          def initialize_dependencies
            @client = Object.new
            @client.define_singleton_method(:address_search) { |address_string|
              FINN_VALIDATION_DRIVER_TEST_DATA[address_string] or []
            }
          end
        }.new
      }

      it "considers an address invalid if finn returns no results" do
        address = AddressKit::Entities::StreetAddress.new({
          street_name: "FOOBAR",
          house_number: 123
        })
        subject.valid?(address).must_equal false
        subject.code.must_equal FinnValidationDriver::CODE_NO_RESULTS
      end

      it "considers an address invalid if finn returns multiple results" do
        address = AddressKit::Entities::StreetAddress.new({
          street_name: "FORNEBUVEIEN",
          house_number: 10
        })
        subject.valid?(address).must_equal false
        subject.code.must_equal FinnValidationDriver::CODE_NOT_SPECIFIC
      end
    end
  end
end

FINN_VALIDATION_DRIVER_TEST_DATA = {
  "FORNEBUVEIEN 10" => [
    AddressKit::Entities::StreetAddress.new({
      street_name: "FORNEBUVEIEN",
      house_number: 10,
      entrance: "A",
      postal_code: 1366,
      city: "LYSAKER"
    }),
    AddressKit::Entities::StreetAddress.new({
      street_name: "FORNEBUVEIEN",
      house_number: 10,
      entrance: "B",
      postal_code: 1366,
      city: "LYSAKER"
    })
  ],
  "TORGET 12, ASKIM" => [
    AddressKit::Entities::StreetAddress.new({
      street_name: "TORGEIR BJØRNARAAS GATE",
      house_number: 12,
      postal_code: 1807,
      city: "ASKIM"
    }),
    AddressKit::Entities::StreetAddress.new({
      street_name: "TORGET",
      house_number: 12,
      postal_code: 1830,
      city: "ASKIM"
    })
  ]
}

This class only has one dependency, which is a client for a web service. Here I replace it with a stub returning static data.

I'm fairly new with Ruby so there are probably at least a few issues or potholes with this approach that I'm not seeing. Is there a best practice when it comes to writing classes and tests for them? Am I way off?

Answer 1

As Nigel suggested, looking into dependency injection is worthwhile. In the dissenting article linked, DHH only discusses how DI impacts testing but that's not its only (or even primary) benefit; decoupling is really the goal and testing is often the first indicator of coupling between components. That said, considering DI is much more of an architectural decision, and probably somewhat out of scope here.

I would instead like to expand on Nigel's suggestion to use mocks; i think they will make your spec much more focused and resilient.

A good metric for any spec is the signal to noise ratio; something like:

LOC directly interacting with the object-under-test / LOC in the spec 

In the spec above, i count 4 or 5 lines that directly test FinnValidationDriver, and a lot of glue code. You probably have more specs than you're showing that reuse the test data, so the ratio is likely higher than i'm seeing, but even so, i'd consider this a code smell.

Consider this refactored code:

# ...
describe FinnValidationDriver do
  before do
    @driver = FinnValidationDriver.new
    @address = MiniTest::Mock.new
  end
  it "considers an address invalid if finn returns no results" do
    @address.expect(:address_string, "FOOBAR 123")
    Finn.stub(:address_search, []) do                       #1
      @driver.valid?(@address).must_equal false
      @driver.code.must_equal FinnValidationDriver::CODE_NO_RESULTS
    end
  end
  it "considers an address invalid if finn returns multiple results" do
    @address.expect(:address_string, "FORNEBUVEIEN 10")
    Finn.stub(:address_search, ['any', 'two_things']) do    #2
      @driver.valid?(@address).must_equal false
      @driver.code.must_equal FinnValidationDriver::CODE_NOT_SPECIFIC
    end
  end
end

Notes:

1. I haven't run any of this code, so no guarantees of correctness :)
2. It looks like you're using minitest so i've stuck with MiniTest::Mock, however i don't have much experience with that particular mocking framework. Most frameworks will do the job satisfactorily.
3. One thing MiniTest::Mock doesn't seem to provide is the ability to verify that a stubbed method was called (and with the expected params). This may or may not be important to you (line #1)
4. I've not bothered to call @address.verify because the interaction between StreetAddress and ..Driver isn't the focus of the test.
5. I've made an assumption that the ..Driver just checks how many addresses were returned by Finn, so line #2 just stubs an array with any two arbitrary items. If Finn does more than check the count, you may have to return a more realistic array.

Advantages, IMO:

1. No need to maintain test data. Over the life of a project, i've found this is a big win: the less you have to maintain, the more resistant the spec is to change.
2. Not coupled to the internal data representation of StreetAddress.
3. The original spec had intimate knowledge of ..Driver's @client instance variable, which is A Bad Thing.
4. Much better ratio of LOC directly interacting with ..Driver to total LOC.

A few other thoughts:

• Check out this post by Dave Chelimsky arguing against the explicit use of subject in specs.

• Based only on the little i see of the ..Driver interface, the asymmetry between #valid?(address) and #code() may be a smell. Because it takes an address as a parameter, the valid? method makes FinnValidationDriver appear to be stateless, but the code method depends on the ..Driver hanging onto state from the most recent call to valid?. I'm not making a case for the ..Driver being either stateful or stateless, just that the signatures of the two methods are incongruous. If the ..Driver is meant to be used to validate a single StreetAddress instance, then it should probably require it as a constructor param; if not, then hanging on to state between calls to valid? is a bad idea and all state resulting from validation should be returned by valid?.

Answer 2

Ruby lets you override methods on existing objects, so you don't need to derive a new one to override it.

If you change the constructor to take the dependency explicitly as a parameter you don't need your initialize method. see Dependency Injection. Note: If you follow this pattern everywhere you end up with classes either being building blocks or for wiring blocks together. Factories etc. Some people don't like this... so the other option is...

RSpec lets you mock methods on any instance of a specific class... see https://www.relishapp.com/rspec/rspec-mocks/v/2-6/docs/method-stubs/stub-on-any-instance-of-a-class

describe "any_instance.stub" do
  it "returns the specified value on any instance of the class" do
    Object.any_instance.stub(:foo).and_return(:return_value)

    o = Object.new
    o.foo.should eq(:return_value)
  end
end