4

I'm following a TDD approach to building our app, and creating a whole bunch of service objects, keeping models strictly for data management.

Many of the services I've built interface with models. Take for example MakePrintsForRunner:

class MakePrintsForRunner

  def initialize(runner)
    @runner = runner
  end

  def from_run_report(run_report)
    run_report.photos.each do |photo|
      Print.create(photo: photo, subject: @runner)
    end
  end

end

I appreciate the create method could arguably be abstracted into the Print model, but let's keep it as is for now.

Now, in the spec for MakePrintsForRunner I'm keen to avoid including spec_helper, since I want my service specs to be super fast.

Instead, I stub out the Print class like this:

describe RunnerPhotos do

  let(:runner) { double }
  let(:photo_1) { double(id: 1) }
  let(:photo_2) { double(id: 2) }
  let(:run_report) { double(photos: [photo_1, photo_2]) }

  before(:each) do
    @service = RunnerPhotos.new(runner)
  end

  describe "#create_print_from_run_report(run_report)" do

    before(:each) do
      class Print; end
      allow(Print).to receive(:create)
      @service.create_print_from_run_report(run_report)
    end

    it "creates a print for every run report photo associating it with the runners" do
      expect(Print).to have_received(:create).with(photo: photo_1, subject: runner)
      expect(Print).to have_received(:create).with(photo: photo_2, subject: runner)
    end
  end

end

And all goes green. Perfect!

... Not so fast. When I run the whole test suite, depending on the seed order, I am now running into problems.

It appears that the class Print; end line can sometimes overwrite print.rb's definition of Print (which obviously inherits from ActiveRecord) and therefore fail a bunch of tests at various points in the suite. One example is:

NoMethodError:
  undefined method 'reflect_on_association' for Print:Class

This makes for an unhappy suite.

Any advice on how to tackle this. While this is one example, there are numerous times where a service is directly referencing a model's method, and I've taken the above approach to stubbing them out. Is there a better way?

idrysdale
  • 1,551
  • 2
  • 12
  • 23

3 Answers3

0

You don't have to create the Print class, simply use the one that is loaded, and stub it:

describe RunnerPhotos do

  let(:runner) { double }
  let(:photo_1) { double(id: 1) }
  let(:photo_2) { double(id: 2) }
  let(:run_report) { double(photos: [photo_1, photo_2]) }

  before(:each) do
    @service = RunnerPhotos.new(runner)
  end

  describe "#create_print_from_run_report(run_report)" do

    before(:each) do
      allow(Print).to receive(:create)
      @service.create_print_from_run_report(run_report)
    end

    it "creates a print for every run report photo associating it with the runners" do
      expect(Print).to have_received(:create).with(photo: photo_1, subject: runner)
      expect(Print).to have_received(:create).with(photo: photo_2, subject: runner)
    end
  end

end

Edit

If you really need to create the class in the scope of this test alone, you can undefine it at the end of the test (from How to undefine class in Ruby?):

before(:all) do
  unless Object.constants.include?(:Print)
    class TempPrint; end
    Print = TempPrint
  end
end

after(:all) do
  if Object.constants.include?(:TempPrint)
    Object.send(:remove_const, :Print)
  end
end
Community
  • 1
  • 1
Uri Agassi
  • 36,848
  • 14
  • 76
  • 93
  • The Print class isn't loaded, though. I'm explicitly trying to avoid including print.rb as it relies on ActiveRecord so would make the spec test slow. Running your code results in: `NameError: uninitialized constant Print` – idrysdale Feb 11 '14 at 13:25
  • is the `reflect_on_association` method called from your code? – Uri Agassi Feb 11 '14 at 16:26
  • No, it'll be called on within Print somewhere else in the full test suite, as it's simply an `ActiveRecord::Reflection` method – idrysdale Feb 11 '14 at 16:34
  • Ah, interesting, I've never seen that way of removing something. Sadly now the test suite results in `RuntimeError: Circular dependency detected while autoloading constant Print – idrysdale Feb 11 '14 at 16:51
  • And if you make the class creation conditional (`unless Object.constants.include?(:Print)`)? – Uri Agassi Feb 11 '14 at 17:01
0

I appreciate the create method could arguably be abstracted into the Print model, but let's keep it as is for now.

Let's see what happens if we ignore this line.

Your difficulty in stubbing a class is a sign that the design is inflexible. Consider passing an already-instantiated object to either the constructor of MakePrintsForRunner or the method #from_run_report. Which to choose depends on the permanence of the object - will the configuration of printing need to change at run time? If not, pass to the constructor, if so, pass to the method.

So for our step 1:

class MakePrintsForRunner
  def initialize(runner, printer)
    @runner = runner
    @printer = printer
  end

  def from_run_report(run_report)
    run_report.photos.each do |photo|
      @printer.print(photo: photo, subject: @runner)
    end
  end
end

Now it's interesting that we're passing two objects to the constructor, yet @runner is only ever passed to the #print method of @printer. This could be a sign that @runner doesn't belong here at all:

class MakePrints
  def initialize(printer)
    @printer = printer
  end

  def from_run_report(run_report)
    run_report.photos.each do |photo|
      @printer.print(photo)
    end
  end
end

We've simplified MakePrintsForRunner into MakePrints. This only takes a printer at construction time, and a report at method invocation time. The complexity of which runner to use is now the responsibility of the new 'printer' role.

Note that the printer is a role, not necessarily a single class. You can swap the implementation for different printing strategies.

Testing should now be simpler:

photo1 = double('photo')
photo2 = double('photo')
run_report = double('run report', photos: [photo1, photo2])
printer = double('printer')
action = MakePrints.new(printer)
allow(printer).to receive(:print)

action.from_run_report(run_report)

expect(printer).to have_received(:print).with(photo1)
expect(printer).to have_received(:print).with(photo2)

These changes might not suit your domain. Perhaps a runner shouldn't be attached to a printer for more than one print. In this case, perhaps you should take a different next step.

Another future refactoring might be for #from_run_report to become #from_photos, since the report isn't used for anything but gathering photos. At this point the class looks a bit anaemic, and might disappear altogether (eaching over photos and calling #print isn't too interesting).

Now, how to test a printer? Integrate with ActiveRecord. This is your adapter to the outside world, and as such should be integration tested. If all it really does is create a record, I probably wouldn't even bother testing it - it's just a wrapper around an ActiveRecord call.

Andrew Bruce
  • 224
  • 1
  • 6
0

Class names are just constants so you could use stub_const to stub an undefined constant and return a double.

So instead of defining a class in your before(:each) block do this:

before(:each) do
  stub_const('Print', double(create: nil))
  @service.create_print_from_run_report(run_report)
end
Ritchie
  • 1,486
  • 1
  • 15
  • 19