← Back to Upcase

How to refactor a test suite without let?


(Guirec Corbel) #1

Hi,

According to this article, I try to not use let in a test suite. I’m trying to test this class :

class PaintingOrderCreator < OrderCreator
  attr_accessor :painting, :order, :payment, :form, :error_messages,
    :painting_price

  def initialize(painting:, order:, painting_price:, payment:, form:)
    super(payment: payment, form: form, order: order)
    @painting = painting
    @painting_price = painting_price
  end

  def process
    if super
      change_painting_prices
      assign_period_to_painting
      painting.published? ? extend_painting : send_mail_to_administrators
    end
    payment.process if order.amount != 0
  end

  def fill_attributes(attributes)
    form.from_hash(attributes)
    order.painting = painting
    order.user = painting.owner.user
    order.description = painting.title
    order.period_id = attributes['period_id']
    order.amount = PriceCalculator.new(painting_price: painting_price,
                                       period: order.period).price
    apply_coupon(attributes['coupon_code'])
  end

  private

  def apply_coupon(code)
    CouponCalculator.new(order: order, code: code).apply if code
  end

  def assign_period_to_painting
    painting.update_attribute('period', order.period)
  end

  def extend_painting
    PaintingPublisher.new(painting: painting).publish
  end

  def send_mail_to_administrators
    OrderMailer.send_mail_after_creation_to_administrators(order).deliver
  end

  def change_painting_prices
    changing_date = Date.today + order.period.duration.day - 1.day
    painting.current_painting_price.update_attribute('end_date', changing_date)
    painting_price.update_attribute('begin_date', changing_date)
    painting_price.update_attribute('end_date', nil)
  end
end

This class is responsible to create an order. It must to send an email in some case, change the periods for prices, publish the painting, etc…

This is my spec :

require 'spec_helper'

describe PaintingOrderCreator do
  describe "#process" do
    context "when the form are valid" do
      context "when form is saved" do
        it "update the period of the painting" do
          order = build_stubbed(:order)
          painting = build_stubbed(:painting)
          payment = double(process: nil)
          period = double(duration: 1)
          painting_price = double(update_attribute: nil)

          form = double(save: true)

          painting_publisher = double(publish: nil)
          allow(PaintingPublisher).to receive(:new).
            and_return(painting_publisher)

          allow(order).to receive(:period).and_return(period)
          allow(painting).to receive(:current_painting_price).
            and_return(painting_price)

          subject = PaintingOrderCreator.new(painting: painting,
                                             order: order,
                                             painting_price: painting_price,
                                             form: form,
                                             payment: payment)

          allow(subject).to receive(:valid?).and_return(true)

          expect(painting).to receive(:update_attribute).with('period', period)
          subject.process
        end

        context "when the painting is already published" do
          it "extend the publishing date" do
            order = build_stubbed(:order)
            painting = build_stubbed(:painting)
            payment = double(process: nil)
            painting_publisher = double(publish: nil)
            form = double(save: true)
            painting_price = double(update_attribute: nil)
            allow(painting).to receive(:current_painting_price).
              and_return(painting_price)

            allow(PaintingPublisher).to receive(:new).
              and_return(painting_publisher)

            allow(painting).to receive(:update_attribute)
            painting.published = true

            subject = PaintingOrderCreator.new(painting: painting,
                                               order: order,
                                               painting_price: painting_price,
                                               form: form,
                                               payment: payment)
            allow(subject).to receive(:valid?).and_return(true)

            subject.process
            expect(painting_publisher).to have_received(:publish)
          end
        end

        context "when the painting is not published" do
          it "send an email to administrators" do
            order = build_stubbed(:order)
            painting = build_stubbed(:painting, published: false)
            painting_price = double(update_attribute: nil)
            payment = double(process: nil)
            form = double(save: true)
            mailer = double(deliver: nil)

            allow(painting).to receive(:update_attribute)
            allow(painting).to receive(:current_painting_price).
              and_return(painting_price)

            subject = PaintingOrderCreator.new(painting: painting,
                                               order: order,
                                               painting_price: painting_price,
                                               form: form,
                                               payment: payment)

            allow(subject).to receive(:valid?).and_return(true)

            allow(OrderMailer).to \
              receive(:send_mail_after_creation_to_administrators).
              and_return(mailer)

            subject.process
          end
        end

        context "when the painting have more than one painting price" do
          it "set the end date of the current painting price" do
            order = build_stubbed(:order)
            painting = build_stubbed(:painting, published: true)
            painting_publisher = double(publish: nil)
            form = double(save: true)
            payment = double(process: nil)

            allow(PaintingPublisher).to receive(:new).
              and_return(painting_publisher)

            allow(painting).to receive(:update_attribute)

            painting_price = build_stubbed(:painting_price)
            period = order.period

            subject = PaintingOrderCreator.new(painting: painting,
                                               order: order,
                                               painting_price: painting_price,
                                               form: form,
                                               payment: payment)

            allow(painting_price).to receive(:update_attribute)

            make_different_current_painting_price_for(painting)

            new_end_date = Date.today + period.duration.day - 1
            subject.process
            expect(painting.current_painting_price).to \
              have_received(:update_attribute).with('end_date', new_end_date)
          end

          it "set the begin date of the next painting price" do
            order = build_stubbed(:order)
            painting = build_stubbed(:painting, published: true)
            painting_publisher = double(publish: nil)
            form = double(save: true)
            payment = double(process: nil)

            allow(PaintingPublisher).to receive(:new).
              and_return(painting_publisher)

            allow(painting).to receive(:update_attribute)

            painting_price = build_stubbed(:painting_price)
            period = order.period

            subject = PaintingOrderCreator.new(painting: painting,
                                               order: order,
                                               painting_price: painting_price,
                                               form: form,
                                               payment: payment)

            allow(painting_price).to receive(:update_attribute)

            make_different_current_painting_price_for(painting)

            new_begin_date = Date.today + period.duration.day - 1.day
            expect(painting_price).to \
              receive(:update_attribute).with('begin_date', new_begin_date)
            subject.process

          end

          it "set the end date of the next painting price to nil" do
            order = build_stubbed(:order)
            painting = build_stubbed(:painting, published: true)
            painting_publisher = double(publish: nil)
            form = double(save: true)
            payment = double(process: nil)

            allow(PaintingPublisher).to receive(:new).
              and_return(painting_publisher)

            allow(painting).to receive(:update_attribute)

            painting_price = build_stubbed(:painting_price)

            subject = PaintingOrderCreator.new(painting: painting,
                                               order: order,
                                               painting_price: painting_price,
                                               form: form,
                                               payment: payment)

            allow(painting_price).to receive(:update_attribute)

            make_different_current_painting_price_for(painting)

            expect(painting_price).to \
              receive(:update_attribute).with('end_date', nil)
            subject.process
          end
        end

        def make_different_current_painting_price_for(painting)
          current_painting_price = painting.current_painting_price
          allow(painting).to receive(:painting_prices).and_return([double, double])
          allow(painting).to receive(:current_painting_price).
            and_return(current_painting_price)
          allow(current_painting_price).to receive(:update_attribute)
        end
      end
    end
  end

  describe "#fill_attributes" do
    it "fill form from hash" do
      order = build_stubbed(:order)
      painting = build_stubbed(:painting)
      form = double
      attributes = {}
      price_calculator = double(price: nil)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      expect(form).to receive(:from_hash).with(attributes)
      subject.fill_attributes(attributes)
    end

    it "set the order painting" do
      painting = build_stubbed(:painting)
      order = build_stubbed(:order, painting: nil)
      form = double(from_hash: nil)
      price_calculator = double(price: nil)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      subject.fill_attributes({})
      expect(order.painting).to eq painting
    end

    it "set the order user" do
      painting = build_stubbed(:painting)
      order = build_stubbed(:order, user: nil)
      form = double(from_hash: nil)
      price_calculator = double(price: nil)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      subject.fill_attributes({})
      expect(order.user).to eq painting.owner.user
    end

    it "set the order description" do
      painting = build_stubbed(:painting)
      order = build_stubbed(:order, description: nil)
      form = double(from_hash: nil)
      price_calculator = double(price: nil)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      subject.fill_attributes({})
      expect(order.description).to eq painting.title
    end

    it "set the order's period" do
      painting = build_stubbed(:painting)
      order = build_stubbed(:order, period: nil)
      form = double(from_hash: nil)
      price_calculator = double(price: nil)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      subject.fill_attributes({'period_id' => '1'})
      expect(order.period_id).to eq 1
    end

    it "calculate order's amount" do
      painting = build_stubbed(:painting)
      order = build_stubbed(:order, amount: nil)
      form = double(from_hash: nil)

      subject = PaintingOrderCreator.new(painting: painting,
                                         order: order,
                                         painting_price: double,
                                         form: form,
                                         payment: double)

      price_calculator = double(price: 100)
      allow(PriceCalculator).to receive(:new).and_return(price_calculator)
      subject.fill_attributes({})
      expect(order.amount).to eq 100
    end

    context "when there is a coupon" do
      it "apply the coupon" do
        painting = build_stubbed(:painting)
        order = build_stubbed(:order)
        form = double(from_hash: nil)
        price_calculator = double(price: nil)
        allow(PriceCalculator).to receive(:new).and_return(price_calculator)

        subject = PaintingOrderCreator.new(painting: painting,
                                           order: order,
                                           painting_price: double,
                                           form: form,
                                           payment: double)

        code = 'TEST'
        coupon_calculator = double
        expect(CouponCalculator).to receive(:new).
          with(order: order, code: code).
          and_return(coupon_calculator)
        expect(coupon_calculator).to receive(:apply)

        subject.fill_attributes({'coupon_code' => code})
      end
    end
  end
end

Each spec take 20 lines, it’s not dry at all, unreadable and hard to maintain.

What can I do? Should I do more functions to create objects in my spec? Should I change the private methods for public in the class? Should I do subclasses of the first class?

Any tips are welcomed, particularly from @jferris as author of the article.

Thanks!


(Luís Ferreira) #2

I think the first thing you need to do is to extract some classes. When you add “etc…” to the class description, it is definitely doing too much.

After you’ve done that, the tests should be a lot smaller, since the context needed by each of the classes will also be smaller.

Apart from that, you can inject dependencies, instead of stubbing new. Also, subject is not a very good name, IMO, it is very generic.

Other things to take into consideration:

  • Passing a form to the class, I don’t understand why
  • Try to use composition over inheritance (not sure what OrderCreator does, so I can help out much)
  • You have different levels of abstraction on the same method (for instance, process)

(Guirec Corbel) #3

Do you think to create classes PaintingPriceChanger, PaintingPeriodAssigner, OrderInformationAssigner is enough? It will clean a bit but I will stub every classes. Something like this :

painting_price_changer = double(apply: nil)
expect(PaintingPriceChanger).to receive(:new).and_return(painting_price_changer)
#...

It may be clearer but not very shorter.

  • form is passed because it is used in parent class.
  • I agree for composition over ineheritance but I think this is not the main problem.
  • For abstraction, I don’t understand what you want to say. Can give me an example please?

(Joe Ferris) #4

I definitely agree with @zamith - breaking up this class is what you really want, and the tests will become cleaner along the way.

It may be helpful to scan for potential responsibilities, but the best way I’ve found to break up classes is by extracting them one at a time. I also think that inheritance is working against you here - for example, the if super in process jumps out at me as a smell.

Using dependency injection and the Open/Closed Principle may help substantially here. Basically, you need to add new test cases and direct dependencies to this class right now every time you add a new aspect to orders, such as coupons or painting prices. If you refactor to use a pattern that allows adding more behaviors externally, such as Observer or Command, most of the behavior in this class disappears and is replaced by configuration.


(Guirec Corbel) #5

Please, can you give me some article to read on those subjects. A code example would be great!

Thanks!


(Luís Ferreira) #6

Ideally your public API would be composed of high level calls, such as:

def process
  change_painting_prices
  assign_period_to_painting
  deal_with_publish_satus
  process_payment
end

This is much easier to read and to think about. Also, it is on the same level of abstraction, which is pretty high. You should try not to mix this high level calls with low level details such as conditionals and checking values, etc…

Hope it helps.


(Guirec Corbel) #7

I agree. I will change it. The specs will stay the same but the code will be more readable.


(Guirec Corbel) #8

To make it clearer, I made this code (I did not test it, this is only theoretical) :

class PaintingOrderCreator < OrderCreator

  attr_accessor :painting, :order, :payment, :form, :error_messages,
    :painting_price

  def initialize(painting:, order:, painting_price:, payment:, form:)
    super(payment: payment, form: form, order: order)
    @painting = painting
    @painting_price = painting_price
  end

  def process
    if super
      change_painting_prices
      assign_period_to_painting
      deal_with_publish_status
    end
    process_to_payment
  end

  def fill_attributes(attributes)
    fill_form(attributes)
    fill_order(attributes)
  end

  private

  def fill_order(attributes)
    OrderInformationFiller.new(order: order, paiting: painting, 
      painting_price: painting_price).fill(attributes)
  end

  def fill_form(attributes)
    form.from_hash(attributes)
  end

  def deal_with_publish_status
    painting.published? ? extend_painting : send_mail_to_administrators
  end

  def process_to_payment
    payment.process if order.amount != 0
  end

  def assign_period_to_painting
    PaintingPeriodAssigner.new(painting: painting, order: order).process
  end

  def extend_painting
    PaintingPublisher.new(painting: painting).publish
  end

  def send_mail_to_administrators
    OrderMailer.send_mail_after_creation_to_administrators(order).deliver
  end

  def change_painting_prices
    PaintingPriceChanger.new(order: order, 
      paiting: painting, 
      painting_price: painting_price).process
  end
end

class OrderInformationFiller
  attr_accessor :painting, :order, :painting_price

  def initialize(order:, painting:, painting_price:)
    @painting = painting 
    @order = order
    @painting_price = painting_price
  end

  def fill(attributes)
    order.painting = painting
    order.user = painting.owner.user
    order.description = painting.title
    order.period_id = attributes['period_id']
    order.amount = PriceCalculator.new(painting_price: painting_price,
                                       period: order.period).price
    apply_coupon(attributes['coupon_code'])
  end

  def apply_coupon(code)
    CouponCalculator.new(order: order, code: code).apply if code
  end
end

class PaintingPriceChanger
  attr_accessor :painting, :order, :painting_price

  def initialize(order:, painting:, painting_price:)
    @painting = painting 
    @order = order
    @painting_price = painting_price
  end

  def process
    changing_date = Date.today + order.period.duration.day - 1.day
    painting.current_painting_price.update_attribute('end_date', changing_date)
    painting_price.update_attribute('begin_date', changing_date)
    painting_price.update_attribute('end_date', nil)
  end
end

class PaintingPeriodAssigner
  attr_accessor :painting, :order

  def initialize(order:, painting:, painting_price:)
    @painting = painting 
    @order = order
  end

  def process
    painting.update_attribute('period', order.period)
  end
end 

And this is a part of tests :

require 'spec_helper'

describe PaintingOrderCreator do
  describe "#process" do
    context "when form is saved" do
      it "update the period of the painting" do
        painting_price_changer = double(process: nil)
        allow(PaintingPriceChanger).to receive(:new).and_return(painting_price_changer)

        painting_publisher = double(publish: nil)
        allow(PaintingPublisher).to receive(:new).and_return(painting_publisher)

        painting = double(published?: false)
        order = double(amount: 0)

        creator = PaintingOrderCreator.new(painting: painting, order: order, 
          painting_price: double, payment: double, form: form)

        painting_period_assigner = double
        allow(PaintingPeriodAssigner).to receive(:new).
          with(painting: painting, order: order).
          and_return(painting_period_assigner)

        expect(painting_period_assigner).to receive(:process)

        creator.process
      end
    end
  end

  it "fill order attributes" do
    painting = double
    order = double
    painting_price = double
    order_information_filler = double
    attributes = double
    form = double(from_hash: nil)

    allow(OrderInformationFiller).to receive(:new).
      (order: order, paiting: painting, painting_price: painting_price).
      and_return(order_information_filler)

    creator = PaintingOrderCreator.new(painting: painting, order: order, 
      painting_price: painting_price, payment: double, form: form)

    expect(order_information_filler).to receive(:fill).with(attributes)

    creator.process
  end
end

I think production and test code are a little bit clearer but the tests are always long and hard to read. There is a lot of scrap that is not very important for the test. If I look a the title “update the period of the painting” I understand what it should do but I must to read the code attentively to understand what it did.

All this scrap can be placed in a before block to be more focused but I think this is not the recommanded way. Maybe a test suite that is hard to read is a symptom of a code smell but I can’t see it.

Any idea to go deeper?


(Joe Ferris) #9

Extracting some methods will definitely clear up the specs. Except for very simple tests, I tend to extract methods for building the SUT, as well as stubs and other dependencies.

There are chapters (including example code!) on dependency injection as well as the open/closed principle in Ruby Science: https://learn.thoughtbot.com/books/13-ruby-science


(Guirec Corbel) #10

Thanks @jferris. Maybe SOLID principles can be a good subject for a weekly iteration.


(Will Greenway) #11

I also write a lot of setup methods and think that they could really help understandability of the test. As a small example, pulling the code to stub out the calls to “new” out of the test could be something like:

def stub_new_price_changer
  price_changer = double(process: nil)
  allow(PaintingPriceChanger).to receive(:new).and_return(price_changer)
  price_changer
end

So that in your test you just have

painting_price_changer = stub_new_price_changer

Like Joe said, having a method to build the system under test with all necessary collaborators can also make each test more readable, and it isolates your dependency on the constructor to a single point instead of having that spread throughout every test.

I would also be careful about stubbing out objects or return values that are irrelevant to the test that you are running. Looking in your first test, the “order” double is stubbed to return “0” for the amount. Is this important? Since the description does not mention price, and no assertions have anything to do with price, I am guessing this is probably an unnecessary over-specification. Someone else (or you in the future) coming in and reading this test can’t immediately tell which elements are important to the outcome of the test. Roy Osherove talks about this and some strategies for avoiding it in his book and talks at The Art of Unit Testing if you can tolerate examples in .NET.


(Guirec Corbel) #12

I refactored my tests and my code. You can find the spec here and the main class here. The commit is here.

I think it’s better but not perfect. There is to many lines to stub object which go to the initializer. May be than I have to many arguments in that initializer but I don’t know how to do else.

Another problem is than I have setup methods that return object which can be not used in the test. I think it’s garbage. Forthermore, I stub thinks which are not necesserly used in spec in the setup methods. I think one of the interest to not use let is to avoid the kind of things. Am I right?

Do you have more advice to improve my code?

Thanks for all you help!


(Will Greenway) #13

I definitely think that everything is more readable now - looking good.

I’m interested to see what other people think, but there is one other thing that jumps out at me. The first few lines of each test are basically just setting up the collaborator doubles to be used in the constructor.

painting = build_stubbed(:painting)
order = build_stubbed(:order)
form = double(save: true)
payment = double(process: nil)

subject = PaintingOrderCreator.new(painting: painting,
                                   order: order,
                                   painting_price: double,
                                   form: form,
                                   payment: payment)

These variables are not really needed for the test, though. So I might pull out a method that builds a valid PaintingOrderCreator, then if I need to use a double to create a specific scenario or to make assertions on, the fact that these properties are available via attr_accessor makes that possible. So a test could look like:

  it "change painting price" do
    painting_price_changer = stub_painting_price_changer
    stub_painting_period_assigner
    stub_painting_publisher

    subject = build_valid_painting_order_creator

    expect(painting_price_changer).to receive(:process)
    subject.process
  end

  context "when the painting is published" do
    it "extend the publishing date" do
      stub_painting_price_changer
      stub_painting_period_assigner
      painting_publisher = stub_painting_publisher

      subject = build_valid_painting_order_creator
      subject.painting = build_stubbed(:painting, published: true)

      expect(painting_publisher).to receive(:publish)
      subject.process
    end
  end

  def build_valid_painting_order_creator
    painting = build_stubbed(:painting)
    order = build_stubbed(:order)
    form = double(save: true)
    payment = double(process: nil)

    PaintingOrderCreator.new(painting: painting,
                             order: order,
                             painting_price: double,
                             form: form,
                             payment: payment)
  end

By doing this you are avoiding declaring variables within the test that do not get used. I think that the main problem with let or before blocks is that you don’t know the “state of the world” when the test starts without finding all of the blocks that will run before it. By splitting out these setup methods and naming them well, you can get a sense of what is happening just by reading, and if you’re interested in knowing more about what each setup method does, it’s easy to jump in and find out.

Now, these suggestions are purely to help with readability of the tests. If you are still feeling significant friction in getting a test set up that will pass, that probably points to still needing to break apart the class a little more.


(Luís Ferreira) #14

I think the main problem is not the variable declaration at the beginning of the test, is the fact that you have a constructor (or any method, really) that takes 5 arguments. That’s a lot, and probably a hint that it is still doing too much.

Having a helper methods to create objects is fine, but make sure it is not form the wrong reasons. If you’re doing it just because the initialization is very big, you might be sweeping it under the rug.


(Luís Ferreira) #15

I think it is much nicer.

I still worry that it gets 5 arguments, even that 3 of them are just to send to the parent, which seems odd.

And there are a lot of hard coded class names, which is a form of coupling and it would be better to inject them.

But looking at the first version and this one, it has greatly improved, I think.


(Guirec Corbel) #16

Maybe to check the controller where the class is instanciate can be useful. It’s this one.

All objects passed in argument are builded in the controller. I don’t want to use find methods and params in services because I think it’s not the right place. Most of them can be builded only with params.

Maybe I can do a object builder. I never seen this kind of things. I can do this kind of things

class PaintingOrderBuilder
  def painting
    @painting ||= Painting.unscoped_find(params[:painting_id])
  end
  #...
end

p = PaintingOrderBuilder.new(params)
creator = PaintingOrderCreator.new(builder: p)

Maybe I can do it in a presenter but I think presenter is targeted for views. Right?

I agree with @zamith for hard coupling. I don’t really know what to do.

An Idea can be to make all my sevices respond to call and do something like this :

notifiers = [PaintingPublisher.new(...), PaintingPeriodAssigner.new(...), PaintingPriceChanger.new(...)]
creator.services = notifiers

class PaintingOrderCreator
  #...
  def call_all_services
    sevices.each(&:call)
  end
end

I’m not sure.

What you think?


(Luís Ferreira) #17

I would probably pass in the params and move the find to the appropriate classes. I’m not very fond of a controller that knows how to construct all kinds of models, such as Painting, PaintingPrice, OrderPayment, etc…

I think controllers should take care of parsing the request, performing an action and deal with the way they should respond. The calling of that action, ideally would be a call to a service that hides all the complexity. You have 10 private methods for one controller action, that is not very good, imo.


(Guirec Corbel) #18

Ok but I need to use form in the view. form need to have painting, order and painting_price. I know I can do this :

def form
  @form ||= creator.form
end
helper_method :form 

But I think it’s a little bit wired. What you think?


(Luís Ferreira) #19

You only need the form when something goes wrong, right? Maybe you can have a creator.form_with_errors.


(Guirec Corbel) #20

No, I need it in the view.

I do this :

<%= default_form(form, ...) do |f| %>
   <%= f.input .... %>
<% end %>