1
\$\begingroup\$

I’m still relatively new to the TDD scene. I’m writing a Rails app, and some duplication immediately sticks out at me in my first model spec. How else can I approach this?

require 'spec_helper'

describe TimeEntry do

  describe "without a project" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "should not be valid" do
      entry.project = nil
      expect(entry).to_not be_valid
    end
  end

  describe "without a user" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "should not be valid" do
      entry.user = nil
      expect(entry).to_not be_valid
    end
  end

end
\$\endgroup\$
1
  • \$\begingroup\$ Your inner describe blocks should be context blocks, semantically. \$\endgroup\$
    – Kyle Smith
    Commented Sep 17, 2014 at 13:19

2 Answers 2

3
\$\begingroup\$

I don't think this could get much drier. let is not cached across examples but it is cached within examples, so you can't move that let out of the nested describe blocks.

Maybe it might be easier for you to define tests that test when the model is valid, and then have a catch-all test to fail in any other case? Unless you are testing validation messages (which I don't think you need to be testing individually, and that is out of the scope of a unit test), you don't need to know which individual field failed, only that the model is invalid and X should happen.

describe TimeEntry do
  describe "validity" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "is invalid without a project" do
      entry.project = nil
      expect(entry).to_not be_valid
    end
    it "is invalid without a user" do
      entry.user = nil
      expect(entry).to_not be_valid
    end
  end
end

Alternatively you can do the above.

\$\endgroup\$
1
  • \$\begingroup\$ He can move the let up, it will work just fine, for exactly the reasons you said. \$\endgroup\$
    – Kyle Smith
    Commented Sep 17, 2014 at 13:18
2
\$\begingroup\$

Dan's answer is great. I'd add a control test, to make sure that the entry object is actually valid by default, e.g.:

it "is valid with a user and project" do
  expect(entry).to be_valid
end

Otherwise, your following tests may be false positives. If the entry was invalid to begin with, the tests prove nothing. It also serves to test your FactoryGirl code.

You could also take a look at the Shoulda gem, which simplifies validation/association testing. Here's an example from their readme:

describe Post do
  it { should belong_to(:user) }
  it { should validate_presence_of(:title) }
end

In general though, expect some duplication-looking-code in tests. For one, you may be testing two very similar-but-not-quite-identical situations. If you were testing manually, you'll also be repeating a lot of steps if you're being thorough. But your tests should also be pretty self-contained, so while it's tempting to structure your code for reuse and maximum DRYness, you actually want to structure it so any single test can be run entirely on its own. Lastly, you just don't want to get too clever in your tests. Of course they should be well-written, but you can end up writing a lot of clever code for you tests - which you then need to test. You can see where that'll lead.

As Dan said, your current code is fine, and can't really get much drier.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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