← Back to Upcase

Is this a right way to write a model spec for a team?


(Charlieanna) #1

Is this a correct use of context? Also should the it “” description be more specific with the number of members or is this fine ? Or should I move this method in the team_activity class which has a unique team and a challenge. A team cannot take part in the same challenge(same name) twice.

require 'spec_helper'

describe Team do
  it { should have_many(:members)}
  it { should have_many(:memberships)}
  it { should belong_to(:admin)}
  it { should validate_presence_of(:name)}
  it { should have_many(:team_activities)}
  it { should have_many(:challenges)}
  it { should respond_to(:slogan)}
  it { should have_attached_file(:photo) }
  it { should respond_to(:goal_for_challenge)}

  describe "#goal_for_challenge" do
  	context "all members have joined before the challenge was choosen" do 
	  	it "should return the goal for the team by calculating number of members multiplied by the number of steps" do
		  	team = create(:team)
		  	challenge = create(:challenge)
		  	team_goal = team.goal_for_challenge(challenge)
		  	expect(team_goal).to eq 0
	    end
    end
    context "some members joined after the challenge started and some before" do
    	it "should return the goal for the team by calculating number of members multiplied by the number of steps" do
		  	team = create(:team)
		  	challenge = create(:challenge)
		  	team_goal = team.goal_for_challenge(challenge)
		  	expect(team_goal).to eq 0
	    end
    end
    context "all members joined after the challenge started" do
    	it "should return the goal for the team by calculating number of members multiplied by the number of steps" do
		  	team = create(:team)
		  	challenge = create(:challenge)
		  	team_goal = team.goal_for_challenge(challenge)
		  	expect(team_goal).to eq 0
	    end
    end
  end
end

(Holger Frohloff) #2

Perhaps you can make the contexts more readable by structuring them this way:

context 'before challenge was chosen'

context 'before challenge started'

context 'after challenge started'

Your it´s all have the same text and you specify the how of the method. I would recommend against that. Just write it 'returns the team´s goal'. If you want to know how it´s done, look inside the class. If you change the implementation details some time into the future, you would have to adjust the text inside your test. I find that´s a tedious task to do.
There are other things you can refactor here but it looks as if you aren´t finished writing this test? Perhaps you can show the final test, then we can give more feedback?

Does that make sense?


(Charlieanna) #3

Thanks :). Yeah I had just started to write it. This is what I came up with one of the scenarios. Didnt take in your suggestions yet.

 context "all members have joined before the challenge was choosen" do 
  it "should return the goal for the team by calculating number of members multiplied by the number of steps" do
    UserActivity.any_instance.stub(:start_time).and_return(Time.now)
    team = create(:team)
    challenge = create(:challenge)
    users = create_list(:user,10)
    team.members << users
    team_activity = create(:team_activity,team:team, challenge:challenge)
    team_goal = team_activity.goal
    expect(team_goal).to eq 0
  end
end

 def goal
    total_goal = 0
    team.members.each do |member|
      user_activity = UserActivity.find_by(user:member, challenge:challenge)
      start_date_of_user_activity = user_activity.start_time
      difference_between_the_start_days = start_date_of_team_activity.day - start_date_of_user_activity.day
      individual_goal = challenge.number_of_steps * difference_between_the_start_days
      total_goal += individual_goal
    end
    return total_goal
  end

(Luís Ferreira) #4

It’s kind of hard to read code here, but three things that I’ve noticed right away:

  1. You are creating all your objects on the database by using factory girl’s create method, you should try to use build_stubbed.
  2. You are creating 10 users to test something, if you really need more than one different user, try to do it with 2. If you can just pretend to have a lot, and not actually create them, even better. The more the records/objects the slower your tests.
  3. You could use timecop instead of stubbing the Time

I believe your code will not work immediately if you follow these ideas (especially number 1), but it will definitely push the code towards a better architecture.

You should also be on the lookout for code smells and possibilities of refactoring. Look at ruby science for a great resource on the topic.

Hope it helps.


(Charlieanna) #5

Thanks. :smile:


(Geoff Harcourt) #6

+1 on both build_stubbed and timecop. Stubbing time on your own is just asking for hurt later.


(Charlieanna) #7

This is the factories.rb I have written till now. It took me ages to come up with this version.

FactoryGirl.define do
  factory :user do
    user_name "ankur kothari"
    sequence(:email) { |n| "person#{n}@example.com" }
    password "12345678"
    password_confirmation "12345678"
  end

  factory :challenge do
    name "Walking Challenge"
    number_of_steps 150000
    number_of_days 30
    visibility "Public"
  end

  factory :membership do
    user
    team
  end

  factory :user_activity do
    start_time  { (Time.now + 3.days) }
    user
    challenge
    status "Participating"
    factory :user_activity_with_daily_activity do

      after(:create) do |user_activity, evaluator|
        create_list(:daily_activity, user_activity.challenge.number_of_days,user_activity: user_activity,start_time:Time.now)
      end
    end
  end

  factory :daily_activity do
    sequence(:steps_covered,0) {|n| n*1000 }
  end


  factory :team do
    name "Getfitgo"
    association :admin, factory: :user
    factory :team_with_members do

      after(:create) do |team, evaluator|
        create_list(:membership, 5,team: team)
      end
    end
  end

  factory :team_activity do
    start_time { (Time.now + 1.days) }
    team 
    challenge
    after(:create) do |team_activity|
      team_activity.team.members.each do |member|
        create(:user_activity_with_daily_activity, user:member,challenge:team_activity.challenge,start_time:team_activity.start_time)
      end
    end
  end
end

(Luís Ferreira) #8

The only problem you might have is with the after(:create) which I’m not sure will be ran with build_stubbed. I believe, however, that there’s an after(:build).