Starkers Starkers - 3 months ago 9
Ruby Question

DRY vs repetitive feature spec

I've created a spec that tests how a user confirms his account. It tests the user in the following scenarios:


  • can sign up

  • can confirm his account if he gives the correct key

  • cannot confirm his account if not signed in

  • cannot confirm his account if his key is invalid

  • cannot confirm his account if his key belongs to someone else.



While this code below is completely untested (wrote it all in the browser) I don't see why it wouldn't work. However, these are my concerns:


  • It gets pretty hard to follow. I'm just testing a few scenarios here, and there are dozens more

  • The indentation will get pretty insane as a result (5 or 7 nested scenarios)

  • So how should I get around this? Should a feature spec actually not be so DRY?

  • Should a feature spec actually be somewhat repetitive? Should I do away with nested scenarios? Should each scenario be on the same 'level' with each scenario using its own, self contained before block that does occasionally have identical code to that in other before blocks? Doing this would be less DRY but more readable and easier to adapt.



Again, this code has never even seen the light of sublime text so there may be some niggles:

DRYer but harder to follow:

describe 'user spec', js: true do
before { visit root_path }
let(:user){ FactoryGirl.build(:user) }

module Helpers
def deliveries
ActionMailer::Base.deliveries
end

def last_email
deliveries.last
end

def sent_emails_count
deliveries.count
end

def reset_action_mailer
deliveries = []
end

def reload_user(user_name)

User.find_by(user_name: user_name) # like to know how to write a method that could find by any attribute, not just username. E.g. `reload_user(email: 'thisismyemail@gotmail.com)`
end

def register_user(user)
click_link 'sign up today!'
fill_in 'user_user_name', with: user.user_name
fill_in 'user_email', with: user.email
fill_in 'user_password', with: user.password
fill_in 'user_password_confirmation', with: user.password_confirmation
click_button 'Register'
end

def sign_out(user) # I have a 'useless' user parameter in there to make code more self-documenting. E.g. sign_out(user_1), sign_out(user_2)
click_link 'sign out'
end

def sign_in(user)
click_link 'sign_in'
fill_in 'user_email', with: user.user_name
fill_in 'user_password', with: user.password
end
end

# shared examples

shared_examples_for 'a succeeded confirmation attempt' do |user|
describe 'the page' do
subject { page }

it_should_behave_like 'profile_page'
it{ should have_selector '.alert-success', text: "Thankyou for confirming! You're now a fully fledged user!" }
it{ should have_selector 'h4', text: 'confirmed' }
end

describe 'the instance' do
it_should_behave_like 'an confirmed instance', user
end
end

shared_examples_for 'a failed confirmation_attempt' do |user|
describe 'the page' do
subject { page }

it_should_behave_like 'profile_page'

it{ should have_selector '.alert-error', text: "Sorry, that key is wrong!" }
it{ should have_selector 'h4', text: 'unconfirmed' }
end



describe 'the instance' do
it_should_behave_like 'an unconfirmed instance', user
end
end

shared_examples_for 'an unconfirmed instance' do |user|
subject { reload_user(user.user_name) }

its(:confirmed_at){ should be_nil }
its(:state){ should eq 'unconfirmed' }
end

shared_examples_for 'a confirmed instance' do |user|
subject { reload_user(user.user_name) }

its(:confirmed_at){ should eq Time.now }
its(:state){ should eq 'confirmed' }
end


scenario 'user signs up' do
before do
register_user(user)
end

describe 'the page' do
subject { page }

it_should_behave_like 'profile_page' # shared example that tests content and title.
it{ should have_selector '.alert-success', text: "welcome to the site, #{user.user_name}! Please check your inbox for a confirmation email. You won't be able to do anything untill you confirm your account!" }
it{ should have_selector 'h4', text: user.email }
it{ should have_selector 'h4', text: user.user_name }
it{ should have_selector 'h4', text: 'unconfirmed' }

end


describe 'the instance' do
subject { User.last } #is this the best way to do this? I want to do `user.reload` but I can't reload user because the user is built in memory (doesn't go through ActiveRecord), and so doesn't have an id

its(:confirmation_key_created_at){ should eq Time.now }
its(:confirmation_key){ should be_base64 } # my own matcher. No, I'm not an autistic maths genius, just testing it's 64 characters long ;)
its(:state){ should eq 'unconfirmed' }
end

describe 'last email' do
subject { last_email }
# using email-spec matchers https://github.com/bmabey/email-spec

it { should have_subject "Hi #{user.user_name}, confirmation link enclosed!" }
it { should deliver_to user.email }
it { should have_body_text user.confirmation_key }

end

scenario 'then attempts confirmation by following the confirmation link in his email' do
before do
visit culminate_path(user.confirmation_key) # In order to closer emulate what the user would do, I would like to actually get the last email and use capybara to click the confirmation link, rather then emulating the click as I'm doing here. How could I do this?
end

it_should_behave_like 'a succeeded confirmation attempt'


end



scenario 'then attempts confirmation by following a made up link' do
let(:user_2) { FactoryGirl.build(:user) }

before do
sign_out(user)
register_user(user_2) # could use FactoryGirl.create but this is an integration test. I want to emulate real-world events as much as possible
sign_out(user_2)
sign_in(user)
visit culminate_path(SecureRandom.urlsafe_base64)
end

it_should_behave_like 'a failed confirmation_attempt'

end

scenario 'then attempts confirmation by following a confirmation link for a different user' do
let(:user_2) { FactoryGirl.build(:user) }

before do
sign_out(user)
register_user(user_2) # could use FactoryGirl.create but this is an integration test. I want to emulate real-world events as much as possible
sign_out(user_2)
sign_in(user)
visit culminate_path(user_2.confirmation_key)
end

it_should_behave_like 'a failed confirmation_attempt'

end

scenario 'then signs out and attempts confirmation by following the confirmation link in his email' do
let(:user_2) { FactoryGirl.build(:user) }

before do
sign_out(user)
visit culminate_path(user.confirmation_key)
end

describe 'the page' do
subject { page }

it_should_behave_like 'sign_in_page'

it{ should have_selector '.alert-error', text: "Sorry, you need to be signed in inorder to attempt confirmation!" }
end


describe 'the instance' do
it_should_behave_like 'a confirmed instance', user
end

scenario 'then signs in and visits link again' do
before do
sign_in(user)
visit culminate_path(user.confirmation_key)
end

it_should_behave_like 'a succeeded confirmation attempt'

end
end
end
end


Easier to follow but not as DRY:

describe 'user spec', js: true do
before { visit root_path }
let(:user){ FactoryGirl.build(:user) }
let(:user_2) { FactoryGirl.build(:user) }

# helper methods and shared examples hidden for brevity

scenario 'user signs up' do
before do
register_user(user)
end

describe 'the page' do
subject { page }

it_should_behave_like 'profile_page' # shared example that tests content and title.
it{ should have_selector '.alert-success', text: "welcome to the site, #{user.user_name}! Please check your inbox for a confirmation email. You won't be able to do anything untill you confirm your account!" }
it{ should have_selector 'h4', text: user.email }
it{ should have_selector 'h4', text: user.user_name }
it{ should have_selector 'h4', text: 'unconfirmed' }

end


describe 'the instance' do
subject { reload_user(user.user_name)} #is this the best way to do this? I want to do `user.reload` but I can't reload user because the user is built in memory (doesn't go through ActiveRecord), and so doesn't have an id

its(:confirmation_key_created_at){ should eq Time.now }
its(:confirmation_key){ should be_base64 } # my own matcher. No, I'm not an autistic maths genius, just testing it's 64 characters long ;)
its(:state){ should eq 'unconfirmed' }
end

describe 'last email' do
subject { last_email }
# using email-spec matchers https://github.com/bmabey/email-spec

it { should have_subject "Hi #{user.user_name}, confirmation link enclosed!" }
it { should deliver_to user.email }
it { should have_body_text user.confirmation_key }

end
end

scenario 'a user signs up and then attempts confirmation by submitting a made up confirmation key' do

before do
register_user(user)
visit culminate_path(SecureRandom.urlsafe_base64)
end

it_should_behave_like 'a failed confirmation_attempt'

end

scenario 'a user signs up and then attempts confirmation by following a confirmation link for a different user' do

before do
register_user(user_2)
sign_out(user_2)
register_user(user)
visit culminate_path(user_2.confirmation_key)
end

it_should_behave_like 'a failed confirmation_attempt'

end

scenario 'then signs in and visits link again' do
before do
sign_in(user)
visit culminate_path(user.confirmation_key)
end

it_should_behave_like 'a succeeded confirmation attempt'
end

scenario 'a user signs out and then attempts confirmation by following the confirmation link in his email' do
before do
register(user)
sign_out(user)
visit culminate_path(user.confirmation_key)
end

describe 'the page' do
subject { page }

it_should_behave_like 'sign_in_page'

it{ should have_selector '.alert-error', text: "Sorry, you need to be signed in inorder to attempt confirmation!" }
end


describe 'the instance' do
it_should_behave_like 'an unconfirmed instance', user
end
end
end

Answer

I think you're right to eliminate most of the duplication. However, I would not use nested scenarios, since they require the reader to jump around in the file. I also wouldn't use shared examples, because they obscure control flow.

I'd do something like this:

describe "user signup and confirmation" do

  let(:user) { # whatever }
  before do
    register_user
    visit root_path
  end

  scenario "user signs up" do
    page_should_show_that_user_is_not_confirmed
    user_should_not_be_confirmed
    # Write assertions here to assert that confirmation email was sent
    confirmation_url = confirmation_url_from_email
    visit confirmation_url
    page_should_show_that_user_is_confirmed
    user_should_be_confirmed
  end

  scenario "user logs out before visiting confirmation URL" do
    # no need to do the same assertions as we did in the previous scenario before the visit
    confirmation_url = confirmation_url_from_email
    sign_out user
    visit confirmation_url
    page_should_show_that_user_is_confirmed
    user_should_be_confirmed
  end

  scenario "user attempts to sign up with an unknown confirmation key" do
    visit culminate_path("some garbage")
    page_should_show_that_user_is_not_confirmed
    user_should_not_be_confirmed
  end

  scenario "user attempts to sign up with the wrong confirmation key" do
    user2 = # whatever
    visit culminate_path(user2.confirmation_key)
    page_should_show_that_user_is_not_confirmed
    user_should_not_be_confirmed
  end

  def page_should_show_that_user_is_not_confirmed
    # Assertions
  end

  def page_should_show_that_user_is_confirmed
    # Assertions
  end

  def user_should_not_be_confirmed
    # Assertions
  end

  def user_should_be_confirmed
    # Assertions
  end

  def confirmation_url_from_email
    # Don't assert anything, just get the URL from the email
  end

end

It's relatively readable overall, and I think there's just enough duplication to maintain readability.