CODEWITHSUNDEEP
rubyrspecrefactoring
Andrey Larin
Andrey Larin

Reputation: 94

How to refactor this rails spec?

Consider this code snippet: 

context 'votes' do
  it { should_not be_able_to :vote_yes, create(:question, user: user), user: user }
  it { should be_able_to :vote_yes, create(:question, user: other_user), user: user }
  it { should_not be_able_to :vote_no, create(:question, user: user), user: user }
  it { should be_able_to :vote_no, create(:question, user: other_user), user: user }
  it { should_not be_able_to :reject_vote, create(:question, user: user), user: user }
  it { should be_able_to :reject_vote, create(:question, user: other_user), user: user }

  it { should_not be_able_to :vote_yes, create(:answer, user: user), user: user }
  it { should be_able_to :vote_yes, create(:answer, user: other_user), user: user }
  it { should_not be_able_to :vote_no, create(:answer, user: user), user: user }
  it { should be_able_to :vote_no, create(:answer, user: other_user), user: user }
  it { should_not be_able_to :reject_vote, create(:answer, user: user), user: user }
  it { should be_able_to :reject_vote, create(:answer, user: other_user), user: user }
end

How to refactor into something with less lines and duplications?

Upvotes: 0

Views: 39

Answers (1)

Tom Lord
Tom Lord

Reputation: 28285

The first thing I would do is split up your tests into more logical groupings, as it's currently very confusing (at a glance) what the expected behaviour is:

context 'voting yes' do
  it { should be_able_to :vote_yes, create(:answer, user: other_user), user: user }
  it { should_not be_able_to :vote_yes, create(:question, user: user), user: user }

  it { should be_able_to :vote_yes, create(:question, user: other_user), user: user }
  it { should_not be_able_to :vote_yes, create(:answer, user: user), user: user }
end

context 'voting no' do
  it { should be_able_to :vote_no, create(:question, user: other_user), user: user }
  it { should_not be_able_to :vote_no, create(:question, user: user), user: user }

  it { should be_able_to :vote_no, create(:answer, user: other_user), user: user }
  it { should_not be_able_to :vote_no, create(:answer, user: user), user: user }
end

context 'rejecting vote' do
  it { should be_able_to :reject_vote, create(:question, user: other_user), user: user }
  it { should_not be_able_to :reject_vote, create(:question, user: user), user: user }

  it { should be_able_to :reject_vote, create(:answer, user: other_user), user: user }
  it { should_not be_able_to :reject_vote, create(:answer, user: user), user: user }
end

Looking at this restructured list of tests, it is much easier to see a clear behavioural pattern. You could remove the repetition as follows:

%i(vote_yes vote_no reject_vote).each do |action_performed|
  context "can #{action_performed} against other users" do
    it { should be_able_to action_performed, create(:question, user: other_user), user: user }
    it { should be_able_to action_performed, create(:answer, user: other_user), user: user }
  end

  context "cannot #{action_performed} against self" do
    it { should_not be_able_to action_performed, create(:question, user: user), user: user }
    it { should_not be_able_to action_performed, create(:answer, user: user), user: user }
  end 
end

You may even be tempted to take this one step further to remove duplication between question and answer tests:

%i(vote_yes vote_no reject_vote).each do |action_performed|
  %i(question answer).each do |record_type|
    it "can #{action_performed} against #{record_type} for other users" do
      should be_able_to action_performed, create(record_type, user: other_user), user: user 
    end

    it "cannot #{action_performed} against #{record_type} for self" do
      should_not be_able_to action_performed, create(record_type, user: user), user: user
    end
  end
end

However, this probably makes the tests harder to understand and edit, so I'd advise against it... Maybe if that list of record types (question, answer, ...) grows much longer, then you could consider an approach like this.

Upvotes: 2

Related Questions