← Back to Upcase

Any tips on how to refactor this?


(Charlieanna) #1

I am getting this error when I run all the specs but not when I run only this spec.

 Challenges Current Challenges are shown on the dashboard of the user
     Failure/Error: expect(page).to have_content "#{challenge.name} started on #{ActiveSupport::Inflector.ordinalize(Time.now.to_date.strftime("%B %d, %Y"))}"
       expected #has_content?("Walking Challenge started on December 31, 1989th") to return true, got false    



scenario "Current Challenges are shown on the dashboard of the user" do
      	challenge = create(:challenge,number_of_steps:50000,number_of_days:7)
        Timecop.travel(Time.now - 1.day)
        visit root_path 
        click_link "Sign up"
        fill_in "Email",with: "ankothari@gmail.com"
        fill_in "User name",with: "Ankur Kothari"
        fill_in "Password",with: "aakk3322"
        click_button "Create Account"
        User.find_by(email: "ankothari@gmail.com").confirm!
        click_link "Create Team"
        fill_in "Name",with: "GetFitGo"
        click_button "Create Team"
        click_link "Choose #{challenge.name}"
        fill_in "Start time",with: Time.now.to_date
        click_button "Start Challenge"
        expect(page).to have_content "#{challenge.name} started on #{ActiveSupport::Inflector.ordinalize(Time.now.to_date.strftime("%B %d, %Y"))}"
      end

Thanks


(Geoff Harcourt) #2

IMHO, building tests that rely on the current time is inherently brittle. Since you’re already using Timecop, I would add a first step in this test (or a before hook) that freezes the time at a pre-set date. Then in your last step, rather than basing your test on a calculated time, I would just have the expectation explicitly look for “April 2nd, 2014” or whatever you are generating there.

Other than potentially being a maintenance headache (tests might change if the travel spans over a change in year and other weird events that sometimes happen), the second reason to refactor your test to use a frozen time is that you will be able to immediately tell from reading the test what the result is supposed to be. Right now it’s not immediately apparent when I read that test what date I should be expecting to see, as I have to go back to your Timecop.travel step and do the math in my head.

(One final suggestion on readability here. I would add line breaks after your Timecop.travel line and before your expect line in order to separate your test setup, exercise, and verification phase. This test is already fairly long, and you’ll get some better readability improvement from explicitly calling out the borders between those three phases with line breaks.)

Hope this helps.


(Charlieanna) #3

something like this?

require 'spec_helper'

feature "Daily Activity" do
  before do
    new_time = Time.local(2008, 9, 1, 12, 0, 0)
    Timecop.freeze(new_time)
  end
  scenario "User can feed in his daily activity" do
    user = create(:user)
    challenge = create(:challenge)
    user_activity = create(:user_activity_with_daily_activity,challenge: challenge, user: user, start_time: Time.now)
    Timecop.travel(Time.now + 1.day)
    visit root_path
    click_link "Login" 
    fill_in "Email",with: user.email
    fill_in "Password",with: user.password
    click_button "Sign in"
    click_link challenge.name
    expect(page).to have_content "Challenge started on September 01, 2008th"
    save_and_open_page
    fill_in "Steps covered",with: 100000
    click_button "Add steps"
    expect(page).to have_content "Thanks for entering your steps"
  end
end

(Geoff Harcourt) #4

I think that’s one clear start toward a clearer, easier to read test.

If you have other tests that are going to require a user to be logged in, I would next refactor those steps into a “and I log in as [username]” so that your steps from click_link "Login" to click_button "Sign in" can all be compressed into one easy to read step and reused elsewhere.

I realize my advice here isn’t helping a ton with your test isolation, but I think it’s going to push your tests in the right direction. Did freezing the time for that test change whether or not it failed when run out of sequence?


(Charlieanna) #5

Freezing the time didnt help actually. I could not understand what is happening. When I run it as a single spec then it passed but not with all the other specs.

Also I now I created this file for the login

# spec/support/features/session_helpers.rb
module Features
  module SessionHelpers
    def sign_up_with(email, password)
      visit sign_up_path
      fill_in 'Email', with: email
      fill_in 'Password', with: password
      click_button 'Sign up'
    end

    def sign_in
      user = create(:user)
      visit new_user_session_path
      save_and_open_page
      fill_in 'Email', with: user.email
      fill_in 'Password', with: user.password
      click_button 'Sign in'
    end

    def sign_in_as(user)
      visit new_user_session_path
      fill_in 'Email', with: user.email
      fill_in 'Password', with: user.password
      click_button 'Sign in'
    end
  end
end