Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new to RSpec and testing in general. I've come up with a spec for testing my Content model and I need some feedback 'cause I think there are many improvements that can be done. I don't know if the way I did it is considered over-testing, bloated/wrong code or something. Here are my files.

app/models/content.rb

class Content < ActiveRecord::Base

  extend FriendlyId
  friendly_id :title, use: [ :slugged, :history ]
  acts_as_mediumable

  delegate :title, to: :category, prefix: true

  # Associations
  belongs_to :category
  has_many :slides, dependent: :destroy

  # Accessible attributes
  attr_accessible :title, :summary, :body, :category_id,
                  :seo_description, :seo_keywords, :seo_title,
                  :unpublished_at, :published_at, :is_draft

  # Validations
  validates :title, presence: true
  validates :body, presence: true
  validates :category, presence: true
  validates :published_at, timeliness: { allow_nil: false, allow_blank: false }
  validates :unpublished_at, timeliness: { allow_nil: true, allow_blank: true, after: :published_at }, :if => "published_at.present?"

  scope :published, lambda { |*args|
    now = ( args.first || Time.zone.now )
    where(is_draft: false).
    where("(published_at <= ? AND unpublished_at IS NULL) OR (published_at <= ? AND ? <= unpublished_at)", now, now, now).
    order("published_at DESC")
  }

  def self.blog_posts
    joins(:category).where(categories: { acts_as_blog: true })
  end

  def self.latest_post
    blog_posts.published.first
  end

  def to_s
    title
  end

  def seo
    meta = Struct.new(:title, :keywords, :description).new
    meta.title = seo_title.presence || title.presence
    meta.description = seo_description.presence || summary.presence
    meta
  end

end

spec/models/content_spec.rb

require 'spec_helper'

describe Content do
  it "has a valid factory" do
    create(:content).should be_valid
  end

  it "is invalid without a title" do
    build(:content, title: nil).should_not be_valid
  end

  it "is invalid without a body" do
    build(:content, body: nil).should_not be_valid
  end

  it "is invalid without a category" do
    build(:content, category: nil).should_not be_valid
  end

  it "is invalid when publication date is nil" do
    build(:content, published_at: nil).should_not be_valid
  end

  it "is invalid when publication date is blank" do
    build(:content, published_at: "").should_not be_valid
  end

  it "is invalid when publication date is malformed" do
    build(:content, published_at: "!0$2-as-#{nil}").should_not be_valid
  end

  # TODO: You shall not pass! (for now)
  # it "is invalid when expiration date is malformed" do
  #   build(:content, unpublished_at: "!0$2-as-#{nil}").should_not be_valid
  # end

  it "is invalid when publication date is nil and expiration date is set" do
    build(:content, published_at: nil, unpublished_at: 3.weeks.ago).should_not be_valid
  end

  it "is invalid when expiration date is before publication date" do
    build(:content, published_at: 1.week.ago, unpublished_at: 2.weeks.ago).should_not be_valid
  end

  it "returns a content's title as a string" do
    content = create(:content)
    content.to_s.should eq content.title
  end

  describe "filters by publication dates" do
    before :each do
      @published_three_weeks_ago = create(:published_three_weeks_ago_content)
      @expiring_in_two_weeks = create(:expiring_in_two_weeks_content)
      @publish_in_tree_weeks = create(:publish_in_tree_weeks_content)
    end

    context "with matching dates" do
      it "returns a sorted array of results that match for current time" do
        Content.published.should include @published_three_weeks_ago, @expiring_in_two_weeks
      end

      it "returns a sorted array of results that match for future time" do
        Content.published(3.weeks.from_now).should include @published_three_weeks_ago, @publish_in_tree_weeks
      end
    end

    context "without matching dates" do
      it "returns an empty array" do
        Content.published(2.months.ago).should eq [ ]
      end
    end
  end

  describe "filters contents by blog category" do
    before :each do
      @blog_category = create(:blog_category)
    end

    context "with matching contents" do
      it "returns only blog posts" do
        one_page = create(:content)
        another_page = create(:content)
        first_post = create(:content, category: @blog_category)
        second_post = create(:content, category: @blog_category)

        Content.blog_posts.should include first_post, second_post
      end
    end

    context "without matching contents" do
      it "returns an empty array" do
        one_page = create(:content)
        another_page = create(:content)

        Content.blog_posts.should eq [ ]
      end
    end
  end

  describe "retrieves latest post" do
    before :each do
      @blog_category = create(:blog_category)
    end

    context "with existing posts" do
      it "return the latest content that belongs to a blog category" do
        first_post = create(:published_three_weeks_ago_content, category: @blog_category)
        second_post = create(:content, published_at: Time.zone.now, category: @blog_category)

        Content.latest_post.should eq second_post
      end
    end

    context "without existing posts" do
      it "returns an nil object" do
        Content.latest_post.should eq nil
      end
    end
  end

  describe "uses seo attributes when present" do
    before :each do
      @it = create(:content)
    end

    context "seo title present" do
      it "returns seo title when present" do
        @it.seo.title.should eq @it.seo_title
      end
    end

    context "seo title non present" do
      it "returns title when seo title is blank" do
        @it.seo_title = ""
        @it.seo.title.should eq @it.title
      end

      it "returns title when seo title is nil" do
        @it.seo_title = nil
        @it.seo.title.should eq @it.title
      end
    end

    context "seo description present" do
      it "returns seo description when present" do
        @it.seo.description.should eq @it.seo_description
      end
    end

    context "seo description non present" do
      it "returns description when seo description is blank" do
        @it.seo_description = ""
        @it.seo.description.should eq @it.summary
      end

      it "returns description when seo description is nil" do
        @it.seo_description = nil
        @it.seo.description.should eq @it.summary
      end
    end
  end
end

spec/factories/contents.rb

FactoryGirl.define do
  factory :content do
    association :category
    title { Faker::Lorem.sentence }
    summary { Faker::Lorem.sentence(10) }
    body { Faker::Lorem.sentence(15) }
    seo_title { Faker::Lorem.sentence }
    seo_description { Faker::Lorem.sentence }
    seo_keywords { Faker::Lorem.words(8).join(", ") }
    published_at { Time.zone.now }
    is_draft { false }

    factory :published_three_weeks_ago_content do
      published_at { 3.weeks.ago }
    end

    factory :expiring_in_two_weeks_content do
      unpublished_at { 2.weeks.from_now }
    end

    factory :publish_in_tree_weeks_content do
      published_at { 3.weeks.from_now }
    end
  end
end

This test is kinda slow but this doesn't bother me that much for now. Any comment is more than welcome.

share|improve this question
    
Note that David Chelimsky advises against the use of an explicit subject: blog.davidchelimsky.net/2012/05/13/… –  Andy Waite Mar 3 '13 at 20:26
add comment

1 Answer

up vote 2 down vote accepted

Nice job. Your tests are nicely compartmentalized. It is indeed good to test the factory independently. Good use of describe and context.

Consider using the shoulda-matchers gem

Tests for many of the rails model associations and validations can be handled by the shoulda-matchers gem. For example, this line:

validates :title, presence: true

can be tested like so:

it {should validate_presence_of(:title)}

Consider using pending

Here's a commented-out test:

# TODO: You shall not pass! (for now)
# it "is invalid when expiration date is malformed" do
#   build(:content, unpublished_at: "!0$2-as-#{nil}").should_not be_valid
# end

Rspec has a method for documenting tests that don't (and can't yet be made to) pass:

it "is invalid when expiration date is malformed" do
  pending
  build(:content, unpublished_at: "!0$2-as-#{nil}").should_not be_valid
end

The nice thing about pending is that it shows up in the test output, making it less easily forgotten than commented-out code. Also, you can give a reason, e.g.:

pending "Can't pass until the vendor fixes library xyz"

For clarity, Consider redoing things the factory did

This test:

context "seo description present" do
  it "returns seo description when present" do
    @it.seo.description.should eq @it.seo_description
  end
end

Relies upon the factory having having set the description, but the factory is a long way from the test. This would be clearer if explicit:

context "seo description present" do
  it "returns seo description when present" do
    @it.seo_description = 'foo bar baz'
    @it.seo.description.should eq @it.seo_description
  end
end

Consider using subject

Some of your test sets a variable in a before block and later tests that variable:

before :each do @it = create(:content) end

context "seo title present" do
  it "returns seo title when present" do
    @it.seo.title.should eq @it.seo_title
  end
end

Instead of assigning to a variable, rspec lets you declare a subject:

subject {create(:content)}

Once you've declared a subject, some snazzy syntax becomes available to you:

its('seo.title') {should == subject.title}

subject.seo_title is a little awkward, so rspec lets you name your subject:

subject(:content) {create(:content)}
its('seo.title') {should == content.title}

Consider using let along with subject

In rspec, let defines a memoized, lazily-evaluated value. When used with subject, this can DRY up a spec:

describe "seo.title" do

  let(:title) {'title'}
  subject {create :content, :title => title, :seo_title => seo_title}

  context 'seo title present' do
    let(:seo_title) {'seo title'}
    its('seo.title') {should eq seo_title}
  end

  context 'seo title missing' do
    let(:seo_title) {nil}
    its('seo.title') {should eq title}
  end

end
share|improve this answer
    
Hey Wayne thanks for the reply. You pointed out some really useful stuff. The let/subject/its seems useful, so I'll have to take a look at the documentation. Cheers ;) –  gtzilgkakis Jan 23 '13 at 12:07
    
One more to get this right. When using subject here: subject {create :content, :title => title, :seo_title => seo_title}, does the seo_title gets evaluated when let(:seo_title) {'seo title'} is executed later on? Normally it would raise an error for using an undefined variable... Also here: its('seo.title') {should eq title} is title referring to subject.title or to the variable title set in the beggining, cause every time I tried something like this its('seo.title') {should eq subject.title} just got that subject is a string. –  gtzilgkakis Jan 23 '13 at 12:33
    
@gtz, Instead of its('seo.title') {should eq subject.title} try specify {subject.seo.title.should eq subject.title}. There are... quirks... with its ability to evaluate an arbitrary string. –  Wayne Conrad Jan 23 '13 at 17:00
    
I see. I'm just starting to discover the ways of RSpec. Thanks a lot for the tips! –  gtzilgkakis Jan 25 '13 at 10:24
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.